[SRU][Trusty][PATCH 0/1] block: fix module reference leak on put_disk() call for cgroups throttle

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[SRU][Trusty][PATCH 0/1] block: fix module reference leak on put_disk() call for cgroups throttle

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1683976

== SRU Justification ==
Due to this bug, the module reference count will leak whenever you write to
/sys/fs/cgroup/blkio/blkio.throttle.*  There is a script in the bug report
to reproduce the bug.

Commit 39a169b62b41 resolves this bug and was added to mainline in
v4.5-rc5.  However, it was not cc'd to stable.

This commit is also needed in Xenial, but Xenial is a clean pick where Trusty needs
a minor backport.  The Xenial SRU will be sent separately.

== Fix ==
commit 39a169b62b415390398291080dafe63aec751e0a
Author: Roman Pen <[hidden email]>
Date:   Tue Feb 9 12:33:35 2016 -0700

    block: fix module reference leak on put_disk() call for cgroups throttle

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.

Roman Pen (1):
  block: fix module reference leak on put_disk() call for cgroups
    throttle

 block/blk-cgroup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[SRU][Trusty][PATCH 1/1] block: fix module reference leak on put_disk() call for cgroups throttle

Joseph Salisbury-3
From: Roman Pen <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1683976

get_disk(),get_gendisk() calls have non explicit side effect: they
increase the reference on the disk owner module.

The following is the correct sequence how to get a disk reference and
to put it:

    disk = get_gendisk(...);

    /* use disk */

    owner = disk->fops->owner;
    put_disk(disk);
    module_put(owner);

fs/block_dev.c is aware of this required module_put() call, but f.e.
blkg_conf_finish(), which is located in block/blk-cgroup.c, does not put
a module reference.  To see a leakage in action cgroups throttle config
can be used.  In the following script I'm removing throttle for /dev/ram0
(actually this is NOP, because throttle was never set for this device):

    # lsmod | grep brd
    brd                     5175  0
    # i=100; while [ $i -gt 0 ]; do echo "1:0 0" > \
        /sys/fs/cgroup/blkio/blkio.throttle.read_bps_device; i=$(($i - 1)); \
    done
    # lsmod | grep brd
    brd                     5175  100

Now brd module has 100 references.

The issue is fixed by calling module_put() just right away put_disk().

Signed-off-by: Roman Pen <[hidden email]>
Cc: Gi-Oh Kim <[hidden email]>
Cc: Tejun Heo <[hidden email]>
Cc: Jens Axboe <[hidden email]>
Cc: [hidden email]
Cc: [hidden email]
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit 39a169b62b415390398291080dafe63aec751e0a)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 block/blk-cgroup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index a717585..640ea87 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -695,6 +695,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 {
  struct gendisk *disk;
  struct blkcg_gq *blkg;
+ struct module *owner;
  unsigned int major, minor;
  unsigned long long v;
  int part, ret;
@@ -706,7 +707,9 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
  if (!disk)
  return -EINVAL;
  if (part) {
+ owner = disk->fops->owner;
  put_disk(disk);
+ module_put(owner);
  return -EINVAL;
  }
 
@@ -722,7 +725,9 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
  ret = PTR_ERR(blkg);
  rcu_read_unlock();
  spin_unlock_irq(disk->queue->queue_lock);
+ owner = disk->fops->owner;
  put_disk(disk);
+ module_put(owner);
  /*
  * If queue was bypassing, we should retry.  Do so after a
  * short msleep().  It isn't strictly necessary but queue
@@ -753,9 +758,13 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep);
 void blkg_conf_finish(struct blkg_conf_ctx *ctx)
  __releases(ctx->disk->queue->queue_lock) __releases(rcu)
 {
+ struct module *owner;
+
  spin_unlock_irq(ctx->disk->queue->queue_lock);
  rcu_read_unlock();
+ owner = ctx->disk->fops->owner;
  put_disk(ctx->disk);
+ module_put(owner);
 }
 EXPORT_SYMBOL_GPL(blkg_conf_finish);
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: [SRU][Trusty][PATCH 0/1] block: fix module reference leak on put_disk() call for cgroups throttle

Kamal Mostafa-2
In reply to this post by Joseph Salisbury-3
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: [SRU][Trusty][PATCH 1/1] block: fix module reference leak on put_disk() call for cgroups throttle

Colin Ian King-2
In reply to this post by Joseph Salisbury-3
On 19/04/17 19:36, Joseph Salisbury wrote:

> From: Roman Pen <[hidden email]>
>
> BugLink: http://bugs.launchpad.net/bugs/1683976
>
> get_disk(),get_gendisk() calls have non explicit side effect: they
> increase the reference on the disk owner module.
>
> The following is the correct sequence how to get a disk reference and
> to put it:
>
>     disk = get_gendisk(...);
>
>     /* use disk */
>
>     owner = disk->fops->owner;
>     put_disk(disk);
>     module_put(owner);
>
> fs/block_dev.c is aware of this required module_put() call, but f.e.
> blkg_conf_finish(), which is located in block/blk-cgroup.c, does not put
> a module reference.  To see a leakage in action cgroups throttle config
> can be used.  In the following script I'm removing throttle for /dev/ram0
> (actually this is NOP, because throttle was never set for this device):
>
>     # lsmod | grep brd
>     brd                     5175  0
>     # i=100; while [ $i -gt 0 ]; do echo "1:0 0" > \
>         /sys/fs/cgroup/blkio/blkio.throttle.read_bps_device; i=$(($i - 1)); \
>     done
>     # lsmod | grep brd
>     brd                     5175  100
>
> Now brd module has 100 references.
>
> The issue is fixed by calling module_put() just right away put_disk().
>
> Signed-off-by: Roman Pen <[hidden email]>
> Cc: Gi-Oh Kim <[hidden email]>
> Cc: Tejun Heo <[hidden email]>
> Cc: Jens Axboe <[hidden email]>
> Cc: [hidden email]
> Cc: [hidden email]
> Signed-off-by: Jens Axboe <[hidden email]>
> (backported from commit 39a169b62b415390398291080dafe63aec751e0a)
> Signed-off-by: Joseph Salisbury <[hidden email]>
> ---
>  block/blk-cgroup.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index a717585..640ea87 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -695,6 +695,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>  {
>   struct gendisk *disk;
>   struct blkcg_gq *blkg;
> + struct module *owner;
>   unsigned int major, minor;
>   unsigned long long v;
>   int part, ret;
> @@ -706,7 +707,9 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>   if (!disk)
>   return -EINVAL;
>   if (part) {
> + owner = disk->fops->owner;
>   put_disk(disk);
> + module_put(owner);
>   return -EINVAL;
>   }
>  
> @@ -722,7 +725,9 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>   ret = PTR_ERR(blkg);
>   rcu_read_unlock();
>   spin_unlock_irq(disk->queue->queue_lock);
> + owner = disk->fops->owner;
>   put_disk(disk);
> + module_put(owner);
>   /*
>   * If queue was bypassing, we should retry.  Do so after a
>   * short msleep().  It isn't strictly necessary but queue
> @@ -753,9 +758,13 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep);
>  void blkg_conf_finish(struct blkg_conf_ctx *ctx)
>   __releases(ctx->disk->queue->queue_lock) __releases(rcu)
>  {
> + struct module *owner;
> +
>   spin_unlock_irq(ctx->disk->queue->queue_lock);
>   rcu_read_unlock();
> + owner = ctx->disk->fops->owner;
>   put_disk(ctx->disk);
> + module_put(owner);
>  }
>  EXPORT_SYMBOL_GPL(blkg_conf_finish);
>  
>
Sane looking upstream fix, backport looks good, good test case. This
does not seem to be tested for Trusty, but going on the Xenial results
I'm OK with that.

Thanks Joe.

Acked-by: Colin Ian King <[hidden email]>



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

APPLIED: [SRU][Trusty][PATCH 0/1] block: fix module reference leak on put_disk() call for cgroups throttle

Thadeu Lima de Souza Cascardo-3
In reply to this post by Joseph Salisbury-3
Applied to trusty master-next branch.

Thanks.
Thadeu Cascardo.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Loading...