[B] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached
diff mbox series

Message ID 20190729191724.16235-1-mfo@canonical.com
State New
Headers show
Series
  • [B] bcache: only set BCACHE_DEV_WB_RUNNING when cached device attached
Related show

Commit Message

Mauricio Faria de Oliveira July 29, 2019, 7:17 p.m. UTC
From: Coly Li <colyli@suse.de>

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 <bjorn.forsman@gmail.com>
Signed-off-by: Coly Li <colyli@suse.de>
Reviewed-by: Bjørn Forsman <bjorn.forsman@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
[mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 drivers/md/bcache/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Connor Kuehl July 29, 2019, 10:49 p.m. UTC | #1
On 7/29/19 12:17 PM, Mauricio Faria de Oliveira wrote:
> From: Coly Li <colyli@suse.de>
> 
> 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 <bjorn.forsman@gmail.com>
> Signed-off-by: Coly Li <colyli@suse.de>
> Reviewed-by: Bjørn Forsman <bjorn.forsman@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> (backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
> [mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>

Acked-by: Connor Kuehl <connor.kuehl@canonical.com>

> ---
>  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);
>  
>
Andrea Righi July 31, 2019, 6:52 a.m. UTC | #2
On Mon, Jul 29, 2019 at 04:17:24PM -0300, Mauricio Faria de Oliveira wrote:
> From: Coly Li <colyli@suse.de>
> 
> 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 <bjorn.forsman@gmail.com>
> Signed-off-by: Coly Li <colyli@suse.de>
> Reviewed-by: Bjørn Forsman <bjorn.forsman@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> (backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
> [mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>

Looks like a sane fix to me.

Acked-by: Andrea Righi <andrea.righi@canonical.com>

> ---
>  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
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Khaled Elmously Aug. 12, 2019, 4:30 a.m. UTC | #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 <colyli@suse.de>
> 
> 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 <bjorn.forsman@gmail.com>
> Signed-off-by: Coly Li <colyli@suse.de>
> Reviewed-by: Bjørn Forsman <bjorn.forsman@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> (backported from commit 1f0ffa67349c56ea54c03ccfd1e073c990e7411e)
> [mfo: backport: fix fuzz; applies with git cherry-pick, not git am]
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> ---
>  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
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch
diff mbox series

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);