| Message ID | 20200110090555.7049-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs: open code log helpers in device_list_add() | expand |
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.
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
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;
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
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);
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(-)