Re: [PATCH] btrfs: don't access possibly stale fs_info data for printing duplicate device

[Nikolay Borisov <nborisov@suse.com>]

On 12.11.20 г. 13:59 ч., Johannes Thumshirn wrote:
> Syzbot reported a possible use-after-free when printing a duplicate device
> warning device_list_add().
> 
> At this point it can happen that a btrfs_device::fs_info is not correctly
> setup yet, so we're accessing stale data, when printing the warning
> message using the btrfs_printk() wrappers.
> 
> Instead of printing possibly uninitialized or already freed memory in
> btrfs_printk(), use a normal pr_warn().
> 
> Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com> , though see below for
a suggestion.

> ---
> 
> This is an alternative and IMHO simpler aproach to what Anand proposed in
> https://lore.kernel.org/linux-btrfs/20200114060920.4527-2-anand.jain@oracle.com/T/
> 
> 
>  fs/btrfs/volumes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..eb1af5e3d596 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -940,8 +940,8 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  			if (device->bdev != path_bdev) {
>  				bdput(path_bdev);
>  				mutex_unlock(&fs_devices->device_list_mutex);
> -				btrfs_warn_in_rcu(device->fs_info,
> -	"duplicate device %s devid %llu generation %llu scanned by %s (%d)",
> +				pr_warn(
> +	"BTRFS: duplicate device %s devid %llu generation %llu scanned by %s (%d)",
>  						  path, devid, found_transid,
>  						  current->comm,
>  						  task_pid_nr(current));
> 


Would a simple 'if()' here catch the case where fs_info is not
initialized essentially open-coding what Anand has proposed? My idea is
to be able to provide the filesystem id when we can (best effort) and
simply use pr_warn otherwise, but without having to change the internals
of btrfs_printk and instead handle the single problematic call site ?