* [PATCH 1/2] btrfs: open code log helpers in device_list_add()
@ 2020-01-10 9:05 Anand Jain
2020-01-10 9:05 ` [PATCH 2/2] btrfs: make the scan logs consistent Anand Jain
2020-01-10 16:42 ` [PATCH 1/2] btrfs: open code log helpers in device_list_add() David Sterba
0 siblings, 2 replies; 6+ messages in thread
From: Anand Jain @ 2020-01-10 9:05 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] btrfs: make the scan logs consistent
2020-01-10 9:05 [PATCH 1/2] btrfs: open code log helpers in device_list_add() Anand Jain
@ 2020-01-10 9:05 ` Anand Jain
2020-01-10 16:42 ` [PATCH 1/2] btrfs: open code log helpers in device_list_add() David Sterba
1 sibling, 0 replies; 6+ messages in thread
From: Anand Jain @ 2020-01-10 9:05 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()
2020-01-10 9:05 [PATCH 1/2] btrfs: open code log helpers in device_list_add() Anand Jain
2020-01-10 9:05 ` [PATCH 2/2] btrfs: make the scan logs consistent Anand Jain
@ 2020-01-10 16:42 ` David Sterba
2020-01-10 23:41 ` Anand Jain
1 sibling, 1 reply; 6+ messages in thread
From: David Sterba @ 2020-01-10 16:42 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()
2020-01-10 16:42 ` [PATCH 1/2] btrfs: open code log helpers in device_list_add() David Sterba
@ 2020-01-10 23:41 ` Anand Jain
2020-01-13 16:25 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2020-01-10 23:41 UTC (permalink / raw)
To: dsterba, linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()
2020-01-10 23:41 ` Anand Jain
@ 2020-01-13 16:25 ` David Sterba
2020-01-14 5:15 ` Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2020-01-13 16:25 UTC (permalink / raw)
To: Anand Jain; +Cc: dsterba, linux-btrfs
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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] btrfs: open code log helpers in device_list_add()
2020-01-13 16:25 ` David Sterba
@ 2020-01-14 5:15 ` Anand Jain
0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2020-01-14 5:15 UTC (permalink / raw)
To: dsterba, linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-14 5:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10 9:05 [PATCH 1/2] btrfs: open code log helpers in device_list_add() Anand Jain
2020-01-10 9:05 ` [PATCH 2/2] btrfs: make the scan logs consistent Anand Jain
2020-01-10 16:42 ` [PATCH 1/2] btrfs: open code log helpers in device_list_add() David Sterba
2020-01-10 23:41 ` Anand Jain
2020-01-13 16:25 ` David Sterba
2020-01-14 5:15 ` Anand Jain
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.