Re: [PATCH 5.4 63/94] btrfs: add dmesg output for first mount and last unmount of a filesystem

[David Sterba <dsterba@suse.cz>]

On Mon, Dec 11, 2023 at 03:56:16PM +0100, Greg Kroah-Hartman wrote:
> On Sat, Dec 09, 2023 at 10:28:36AM -0700, Nathan Chancellor wrote:
> > On Tue, Dec 05, 2023 at 12:17:31PM +0900, Greg Kroah-Hartman wrote:
> > > 5.4-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Qu Wenruo <wqu@suse.com>
> > > 
> > > commit 2db313205f8b96eea467691917138d646bb50aef upstream.
> > > 
> > > There is a feature request to add dmesg output when unmounting a btrfs.
> > > There are several alternative methods to do the same thing, but with
> > > their own problems:
> > > 
> > > - Use eBPF to watch btrfs_put_super()/open_ctree()
> > >   Not end user friendly, they have to dip their head into the source
> > >   code.
> > > 
> > > - Watch for directory /sys/fs/<uuid>/
> > >   This is way more simple, but still requires some simple device -> uuid
> > >   lookups.  And a script needs to use inotify to watch /sys/fs/.
> > > 
> > > Compared to all these, directly outputting the information into dmesg
> > > would be the most simple one, with both device and UUID included.
> > > 
> > > And since we're here, also add the output when mounting a filesystem for
> > > the first time for parity. A more fine grained monitoring of subvolume
> > > mounts should be done by another layer, like audit.
> > > 
> > > Now mounting a btrfs with all default mkfs options would look like this:
> > > 
> > >   [81.906566] BTRFS info (device dm-8): first mount of filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> > >   [81.907494] BTRFS info (device dm-8): using crc32c (crc32c-intel) checksum algorithm
> > >   [81.908258] BTRFS info (device dm-8): using free space tree
> > >   [81.912644] BTRFS info (device dm-8): auto enabling async discard
> > >   [81.913277] BTRFS info (device dm-8): checking UUID tree
> > >   [91.668256] BTRFS info (device dm-8): last unmount of filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> > > 
> > > CC: stable@vger.kernel.org # 5.4+
> > > Link: https://github.com/kdave/btrfs-progs/issues/689
> > > Reviewed-by: Anand Jain <anand.jain@oracle.com>
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > Reviewed-by: David Sterba <dsterba@suse.com>
> > > [ update changelog ]
> > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  fs/btrfs/disk-io.c |    1 +
> > >  fs/btrfs/super.c   |    5 ++++-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > --- a/fs/btrfs/disk-io.c
> > > +++ b/fs/btrfs/disk-io.c
> > > @@ -2829,6 +2829,7 @@ int open_ctree(struct super_block *sb,
> > >  		goto fail_alloc;
> > >  	}
> > >  
> > > +	btrfs_info(fs_info, "first mount of filesystem %pU", disk_super->fsid);
> > 
> > clang tells me this backport does not appear to be correct and will
> > likely introduce a null pointer deference:
> > 
> >   fs/btrfs/disk-io.c:2832:55: warning: variable 'disk_super' is uninitialized when used here [-Wuninitialized]
> >    2832 |         btrfs_info(fs_info, "first mount of filesystem %pU", disk_super->fsid);
> >         |                                                              ^~~~~~~~~~
> >   fs/btrfs/ctree.h:3002:41: note: expanded from macro 'btrfs_info'
> >    3002 |         btrfs_printk(fs_info, KERN_INFO fmt, ##args)
> >         |                                                ^~~~
> >   fs/btrfs/disk-io.c:2630:38: note: initialize the variable 'disk_super' to silence this warning
> >    2630 |         struct btrfs_super_block *disk_super;
> >         |                                             ^
> >         |                                              = NULL
> >   1 warning generated.
> 
> Thanks for the notice, I've now reverted this.

Thanks. I've verified that this only affects 5.4. The patch is still
possible to apply there, the btrfs_info() call would have to be moved
after 'disk_super' is initialized. The patch is not that important so
reverting is OK for me.