[PATCH 1/2] btrfs: open code log helpers in device_list_add()

[PATCH 1/2] btrfs: open code log helpers in device_list_add()

[Anand Jain]

fs_info is born during mount, and operations before the mount such as
scanning and assembling of the device volume should happen without any
reference to fs_info.

However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions leads
to btrfs_printk() which shall print "unknown" instead of sb->s_id. Or
even might UAF as reported in [1].

So do the right thing, don't use btrfs_warn_in_rcu() and
btrfs_info_in_rcu() in device_list_add() instead just open code it.

Link:
[1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6fd90270e2c7..1a419841fc99 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -889,17 +889,21 @@ 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 fsid:devid for %pU:%llu old:%s new:%s",
+				rcu_read_lock();
+				printk_ratelimited(
+		"BTRFS: duplicate device fsid:devid for %pU:%llu old:%s new:%s",
 					disk_super->fsid, devid,
 					rcu_str_deref(device->name), path);
+				rcu_read_unlock();
 				return ERR_PTR(-EEXIST);
 			}
 			bdput(path_bdev);
-			btrfs_info_in_rcu(device->fs_info,
-				"device fsid %pU devid %llu moved old:%s new:%s",
+			rcu_read_lock();
+			printk_ratelimited(
+			"BTRFS: device fsid %pU devid %llu moved old:%s new:%s",
 				disk_super->fsid, devid,
 				rcu_str_deref(device->name), path);
+			rcu_read_unlock();
 		}
 
 		name = rcu_string_strdup(path, GFP_NOFS);
-- 
2.23.0

[PATCH 2/2] btrfs: make the scan logs consistent

[Anand Jain]

Typically we follow, a logging format <parameter> <value> and no ":"
so just follow that here.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
David,
If required you may roll this into the patch 1/2 in this series. I didn't
dare, as there may be some concerns that it isn't relevent there.

 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 1a419841fc99..e6c2bf9d9f53 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -891,7 +891,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 				mutex_unlock(&fs_devices->device_list_mutex);
 				rcu_read_lock();
 				printk_ratelimited(
-		"BTRFS: duplicate device fsid:devid for %pU:%llu old:%s new:%s",
+		"BTRFS: duplicate device fsid %pU devid %llu old %s new %s",
 					disk_super->fsid, devid,
 					rcu_str_deref(device->name), path);
 				rcu_read_unlock();
@@ -900,7 +900,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			bdput(path_bdev);
 			rcu_read_lock();
 			printk_ratelimited(
-			"BTRFS: device fsid %pU devid %llu moved old:%s new:%s",
+			"BTRFS: device fsid %pU devid %llu moved old %s new %s",
 				disk_super->fsid, devid,
 				rcu_str_deref(device->name), path);
 			rcu_read_unlock();
-- 
2.23.0

Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()

[David Sterba]

On Fri, Jan 10, 2020 at 05:05:54PM +0800, Anand Jain wrote:
> fs_info is born during mount, and operations before the mount such as
> scanning and assembling of the device volume should happen without any
> reference to fs_info.
> 
> However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
> fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
> btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions leads
> to btrfs_printk() which shall print "unknown" instead of sb->s_id. Or
> even might UAF as reported in [1].
> 
> So do the right thing, don't use btrfs_warn_in_rcu() and
> btrfs_info_in_rcu() in device_list_add() instead just open code it.
> 
> Link:
> [1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
> Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6fd90270e2c7..1a419841fc99 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -889,17 +889,21 @@ 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 fsid:devid for %pU:%llu old:%s new:%s",
> +				rcu_read_lock();
> +				printk_ratelimited(

Avoiding fs_info here is correct but we don't want to use raw printk or
printk_ratelimited anywhere.

Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()

[Anand Jain]

On 11/1/20 12:42 AM, David Sterba wrote:
> On Fri, Jan 10, 2020 at 05:05:54PM +0800, Anand Jain wrote:
>> fs_info is born during mount, and operations before the mount such as
>> scanning and assembling of the device volume should happen without any
>> reference to fs_info.
>>
>> However the patch commit a9261d4125c9 (btrfs: harden agaist duplicate
>> fsid on scanned devices) used fs_info to call btrfs_warn_in_rcu() and
>> btrfs_info_in_rcu(), so if fs_info is NULL, the stacked functions leads
>> to btrfs_printk() which shall print "unknown" instead of sb->s_id. Or
>> even might UAF as reported in [1].
>>
>> So do the right thing, don't use btrfs_warn_in_rcu() and
>> btrfs_info_in_rcu() in device_list_add() instead just open code it.
>>
>> Link:
>> [1] https://www.spinics.net/lists/linux-btrfs/msg96524.html
>> Fixes: a9261d4125c9 (btrfs: harden agaist duplicate fsid on scanned devices)
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 6fd90270e2c7..1a419841fc99 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -889,17 +889,21 @@ 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 fsid:devid for %pU:%llu old:%s new:%s",
>> +				rcu_read_lock();
>> +				printk_ratelimited(
> 
> Avoiding fs_info here is correct but we don't want to use raw printk or
> printk_ratelimited anywhere.
> 

  I think I discussed this a long time back, that we should rather pass
  fs_devices in btrfs_warn_in_rcu().

  I am ok to make such a change, are you ok? Or I wonder if there is
  any other way?

Thanks, Anand

Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()

[David Sterba]

On Sat, Jan 11, 2020 at 07:41:51AM +0800, Anand Jain wrote:
> >>   			if (device->bdev != path_bdev) {
> >>   				bdput(path_bdev);
> >>   				mutex_unlock(&fs_devices->device_list_mutex);
> >> -				btrfs_warn_in_rcu(device->fs_info,
> >> -			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
> >> +				rcu_read_lock();
> >> +				printk_ratelimited(
> > 
> > Avoiding fs_info here is correct but we don't want to use raw printk or
> > printk_ratelimited anywhere.
> > 
> 
>   I think I discussed this a long time back, that we should rather pass
>   fs_devices in btrfs_warn_in_rcu().
> 
>   I am ok to make such a change, are you ok?

No, this does not sound right at all. Why should be btrfs_warn_in_rcu
special from the other message callbacks? We need to fix one context, so
let's find something less hacky.

>   Or I wonder if there is
>   any other way?

We could add a fs_info stub that will get recognized in btrfs_printk.
Eg.

#define	NO_FS_INFO		((void*)0x1)

btrfs_printk() {

	if (fs_info == NULL)
		devname = "<unknown>";
	else if (fs_info == NO_FS_INFO)
		devname = "...";
	else
		devname = fs_info->sb->sb_id;

Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()

[Anand Jain]

On 14/1/20 12:25 AM, David Sterba wrote:
> On Sat, Jan 11, 2020 at 07:41:51AM +0800, Anand Jain wrote:
>>>>    			if (device->bdev != path_bdev) {
>>>>    				bdput(path_bdev);
>>>>    				mutex_unlock(&fs_devices->device_list_mutex);
>>>> -				btrfs_warn_in_rcu(device->fs_info,
>>>> -			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
>>>> +				rcu_read_lock();
>>>> +				printk_ratelimited(
>>>
>>> Avoiding fs_info here is correct but we don't want to use raw printk or
>>> printk_ratelimited anywhere.
>>>
>>
>>    I think I discussed this a long time back, that we should rather pass
>>    fs_devices in btrfs_warn_in_rcu().
>>
>>    I am ok to make such a change, are you ok?
> 
> No, this does not sound right at all. Why should be btrfs_warn_in_rcu
> special from the other message callbacks? We need to fix one context, so
> let's find something less hacky.
> 
>>    Or I wonder if there is
>>    any other way?
> 
> We could add a fs_info stub that will get recognized in btrfs_printk.
> Eg.
> 
> #define	NO_FS_INFO		((void*)0x1)
> 
> btrfs_printk() {
> 
> 	if (fs_info == NULL)
> 		devname = "<unknown>";
> 	else if (fs_info == NO_FS_INFO)
> 		devname = "...";
> 	else
> 		devname = fs_info->sb->sb_id;
> 

Yeah it makes sense to me. Patches sent.

Thanks, Anand