[B][PATCH] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[B][PATCH] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached

Mauricio Faria de Oliveira-3
From: Coly Li <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1837788

When people set a writeback percent via sysfs file,
  /sys/block/bcache<N>/bcache/writeback_percent
current code directly sets BCACHE_DEV_WB_RUNNING to dc->disk.flags
and schedules kworker dc->writeback_rate_update.

If there is no cache set attached to, the writeback kernel thread is
not running indeed, running dc->writeback_rate_update does not make
sense and may cause NULL pointer deference when reference cache set
pointer inside update_writeback_rate().

This patch checks whether the cache set point (dc->disk.c) is NULL in
sysfs interface handler, and only set BCACHE_DEV_WB_RUNNING and
schedule dc->writeback_rate_update when dc->disk.c is not NULL (it
means the cache device is attached to a cache set).

This problem might be introduced from initial bcache commit, but
commit 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
changes part of the original code piece, so I add 'Fixes: 3fd47bfe55b0'
to indicate from which commit this patch can be applied.

Fixes: 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
Reported-by: Bjørn Forsman <[hidden email]>
Signed-off-by: Coly Li <[hidden email]>
Reviewed-by: Bjørn Forsman <[hidden email]>
Cc: [hidden email]
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
[mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 drivers/md/bcache/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index 17df47d085f1..6acbd38fa946 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -333,8 +333,13 @@ STORE(bch_cached_dev)
  if (attr == &sysfs_writeback_running)
  bch_writeback_queue(dc);
 
+ /*
+ * Only set BCACHE_DEV_WB_RUNNING when cached device attached to
+ * a cache set, otherwise it doesn't make sense.
+ */
  if (attr == &sysfs_writeback_percent)
- if (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
+ if ((dc->disk.c != NULL) &&
+    (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags)))
  schedule_delayed_work(&dc->writeback_rate_update,
       dc->writeback_rate_update_seconds * HZ);
 
--
2.17.1


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

ACK: [B][PATCH] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached

Connor Kuehl
On 7/29/19 12:17 PM, Mauricio Faria de Oliveira wrote:

> From: Coly Li <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1837788
>
> When people set a writeback percent via sysfs file,
>   /sys/block/bcache<N>/bcache/writeback_percent
> current code directly sets BCACHE_DEV_WB_RUNNING to dc->disk.flags
> and schedules kworker dc->writeback_rate_update.
>
> If there is no cache set attached to, the writeback kernel thread is
> not running indeed, running dc->writeback_rate_update does not make
> sense and may cause NULL pointer deference when reference cache set
> pointer inside update_writeback_rate().
>
> This patch checks whether the cache set point (dc->disk.c) is NULL in
> sysfs interface handler, and only set BCACHE_DEV_WB_RUNNING and
> schedule dc->writeback_rate_update when dc->disk.c is not NULL (it
> means the cache device is attached to a cache set).
>
> This problem might be introduced from initial bcache commit, but
> commit 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
> changes part of the original code piece, so I add 'Fixes: 3fd47bfe55b0'
> to indicate from which commit this patch can be applied.
>
> Fixes: 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
> Reported-by: Bjørn Forsman <[hidden email]>
> Signed-off-by: Coly Li <[hidden email]>
> Reviewed-by: Bjørn Forsman <[hidden email]>
> Cc: [hidden email]
> Signed-off-by: Jens Axboe <[hidden email]>
> (backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
> [mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>

Acked-by: Connor Kuehl <[hidden email]>

> ---
>  drivers/md/bcache/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> index 17df47d085f1..6acbd38fa946 100644
> --- a/drivers/md/bcache/sysfs.c
> +++ b/drivers/md/bcache/sysfs.c
> @@ -333,8 +333,13 @@ STORE(bch_cached_dev)
>   if (attr == &sysfs_writeback_running)
>   bch_writeback_queue(dc);
>  
> + /*
> + * Only set BCACHE_DEV_WB_RUNNING when cached device attached to
> + * a cache set, otherwise it doesn't make sense.
> + */
>   if (attr == &sysfs_writeback_percent)
> - if (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
> + if ((dc->disk.c != NULL) &&
> +    (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags)))
>   schedule_delayed_work(&dc->writeback_rate_update,
>        dc->writeback_rate_update_seconds * HZ);
>  
>


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

ACK: [B][PATCH] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached

Andrea Righi
In reply to this post by Mauricio Faria de Oliveira-3
On Mon, Jul 29, 2019 at 04:17:24PM -0300, Mauricio Faria de Oliveira wrote:

> From: Coly Li <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1837788
>
> When people set a writeback percent via sysfs file,
>   /sys/block/bcache<N>/bcache/writeback_percent
> current code directly sets BCACHE_DEV_WB_RUNNING to dc->disk.flags
> and schedules kworker dc->writeback_rate_update.
>
> If there is no cache set attached to, the writeback kernel thread is
> not running indeed, running dc->writeback_rate_update does not make
> sense and may cause NULL pointer deference when reference cache set
> pointer inside update_writeback_rate().
>
> This patch checks whether the cache set point (dc->disk.c) is NULL in
> sysfs interface handler, and only set BCACHE_DEV_WB_RUNNING and
> schedule dc->writeback_rate_update when dc->disk.c is not NULL (it
> means the cache device is attached to a cache set).
>
> This problem might be introduced from initial bcache commit, but
> commit 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
> changes part of the original code piece, so I add 'Fixes: 3fd47bfe55b0'
> to indicate from which commit this patch can be applied.
>
> Fixes: 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
> Reported-by: Bjørn Forsman <[hidden email]>
> Signed-off-by: Coly Li <[hidden email]>
> Reviewed-by: Bjørn Forsman <[hidden email]>
> Cc: [hidden email]
> Signed-off-by: Jens Axboe <[hidden email]>
> (backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
> [mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>

Looks like a sane fix to me.

Acked-by: Andrea Righi <[hidden email]>

> ---
>  drivers/md/bcache/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> index 17df47d085f1..6acbd38fa946 100644
> --- a/drivers/md/bcache/sysfs.c
> +++ b/drivers/md/bcache/sysfs.c
> @@ -333,8 +333,13 @@ STORE(bch_cached_dev)
>   if (attr == &sysfs_writeback_running)
>   bch_writeback_queue(dc);
>  
> + /*
> + * Only set BCACHE_DEV_WB_RUNNING when cached device attached to
> + * a cache set, otherwise it doesn't make sense.
> + */
>   if (attr == &sysfs_writeback_percent)
> - if (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
> + if ((dc->disk.c != NULL) &&
> +    (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags)))
>   schedule_delayed_work(&dc->writeback_rate_update,
>        dc->writeback_rate_update_seconds * HZ);
>  
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

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

APPLIED/cmt: [B][PATCH] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached

Khaled Elmously
In reply to this post by Mauricio Faria de Oliveira-3
Marking thread as applied - this patch was already present in bionic as part of the August 2 bionic stable-update:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1838824


On 2019-07-29 16:17:24 , Mauricio Faria de Oliveira wrote:

> From: Coly Li <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1837788
>
> When people set a writeback percent via sysfs file,
>   /sys/block/bcache<N>/bcache/writeback_percent
> current code directly sets BCACHE_DEV_WB_RUNNING to dc->disk.flags
> and schedules kworker dc->writeback_rate_update.
>
> If there is no cache set attached to, the writeback kernel thread is
> not running indeed, running dc->writeback_rate_update does not make
> sense and may cause NULL pointer deference when reference cache set
> pointer inside update_writeback_rate().
>
> This patch checks whether the cache set point (dc->disk.c) is NULL in
> sysfs interface handler, and only set BCACHE_DEV_WB_RUNNING and
> schedule dc->writeback_rate_update when dc->disk.c is not NULL (it
> means the cache device is attached to a cache set).
>
> This problem might be introduced from initial bcache commit, but
> commit 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
> changes part of the original code piece, so I add 'Fixes: 3fd47bfe55b0'
> to indicate from which commit this patch can be applied.
>
> Fixes: 3fd47bfe55b0 ("bcache: stop dc->writeback_rate_update properly")
> Reported-by: Bjørn Forsman <[hidden email]>
> Signed-off-by: Coly Li <[hidden email]>
> Reviewed-by: Bjørn Forsman <[hidden email]>
> Cc: [hidden email]
> Signed-off-by: Jens Axboe <[hidden email]>
> (backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
> [mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
> ---
>  drivers/md/bcache/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> index 17df47d085f1..6acbd38fa946 100644
> --- a/drivers/md/bcache/sysfs.c
> +++ b/drivers/md/bcache/sysfs.c
> @@ -333,8 +333,13 @@ STORE(bch_cached_dev)
>   if (attr == &sysfs_writeback_running)
>   bch_writeback_queue(dc);
>  
> + /*
> + * Only set BCACHE_DEV_WB_RUNNING when cached device attached to
> + * a cache set, otherwise it doesn't make sense.
> + */
>   if (attr == &sysfs_writeback_percent)
> - if (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
> + if ((dc->disk.c != NULL) &&
> +    (!test_and_set_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags)))
>   schedule_delayed_work(&dc->writeback_rate_update,
>        dc->writeback_rate_update_seconds * HZ);
>  
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

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