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

[David Sterba <dsterba@suse.cz>]

On Fri, Nov 13, 2020 at 05:53:05PM +0100, David Sterba wrote:
> On Thu, Nov 12, 2020 at 12:09:52PM +0000, Johannes Thumshirn wrote:
> > On 12/11/2020 13:03, Nikolay Borisov wrote:
> > > 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 ?
> > > 
> > 
> > Unfortunately not, I've been trying to do that but the device->fs_info pointer
> > exists and accessing it triggers a KASAN splat.
> > 
> > Actually btrfs_printk() is already checking if fs_info is NULL or not to decide
> > whether to print <unknown> or fs_info->sb->s_id.
> > 
> > Another option would be to do btrfs_warn_in_rcu(NULL but that doesn't buy us a lot
> > more.
> 
> It does, not calling pr_warn and using the helpers that print the
> standard header. I really want to avoid using the raw pr_* functions if
> possible, in this case we can use the NULL parameter.
> 
> Previous discussion:
> https://lore.kernel.org/linux-btrfs/20200110090555.7049-1-anand.jain@oracle.com/t/#u
> 
> We can update btrfs_printk to leave out "(device %s)" completely in
> case there's no fs_info, and then switch everything to the helpers (there
> are still too many pr_* left).

--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -240,9 +240,14 @@ void __cold btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, .
        vaf.fmt = fmt;
        vaf.va = &args;
 
-       if (__ratelimit(ratelimit))
-               printk("%sBTRFS %s (device %s): %pV\n", lvl, type,
-                       fs_info ? fs_info->sb->s_id : "<unknown>", &vaf);
+       if (__ratelimit(ratelimit)) {
+               if (fs_info) {
+                       printk("%sBTRFS %s (device %s): %pV\n", lvl, type,
+                                       fs_info->sb->s_id, &vaf);
+               } else {
+                       printk("%sBTRFS %s: %pV\n", lvl, type, &vaf);
+               }
+       }
 
        va_end(args);
 }
---