linux-kernel - Re: linux-next: manual merge of the vfs-brauner tree with the bcachefs tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231026183539.cffe6uljmnjgacxq@moria.home.lan>
Date:   Thu, 26 Oct 2023 14:35:39 -0400
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Stephen Rothwell <sfr@...b.auug.org.au>,
        Christian Brauner <brauner@...nel.org>,
        Kent Overstreet <kent.overstreet@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Next Mailing List <linux-next@...r.kernel.org>,
        Jeff Layton <jlayton@...nel.org>,
        Chuck Lever <chuck.lever@...cle.com>
Subject: Re: linux-next: manual merge of the vfs-brauner tree with the
 bcachefs tree

On Thu, Oct 26, 2023 at 08:16:14AM +0300, Amir Goldstein wrote:
> On Thu, Oct 26, 2023 at 2:02 AM Stephen Rothwell <sfr@...b.auug.org.au> wrote:
> >
> > Hi all,
> >
> > Today's linux-next merge of the vfs-brauner tree got a conflict in:
> >
> >   include/linux/exportfs.h
> >
> > between commit:
> >
> >   85e95ca7cc48 ("bcachefs: Update export_operations for snapshots")
> >
> > from the bcachefs tree and commit:
> >
> >   2560fa66d2ac ("exportfs: define FILEID_INO64_GEN* file handle types")
> >
> > from the vfs-brauner tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging.  You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.
> >
> > --
> > Cheers,
> > Stephen Rothwell
> 
> [adding exportfs maintainers]
> 
> >
> > diff --cc include/linux/exportfs.h
> > index be9900cc8786,21bae8bfeef1..000000000000
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@@ -98,12 -98,17 +98,23 @@@ enum fid_type
> >          */
> >         FILEID_FAT_WITH_PARENT = 0x72,
> >
> >  +      /*
> >  +       * 64 bit inode number, 32 bit subvolume, 32 bit generation number:
> >  +       */
> >  +      FILEID_BCACHEFS_WITHOUT_PARENT = 0x80,
> >  +      FILEID_BCACHEFS_WITH_PARENT = 0x81,
> >  +
> > +       /*
> > +        * 64 bit inode number, 32 bit generation number.
> > +        */
> >  -      FILEID_INO64_GEN = 0x81,
> > ++      FILEID_INO64_GEN = 0x82,
> > +
> > +       /*
> > +        * 64 bit inode number, 32 bit generation number,
> > +        * 64 bit parent inode number, 32 bit parent generation.
> > +        */
> >  -      FILEID_INO64_GEN_PARENT = 0x82,
> > ++      FILEID_INO64_GEN_PARENT = 0x83,
> > +
> 
> This is wrong.
> Those are filesystem defined constants.
> Please don't change them.
> 
> 0x81/0x82 have been used by xfs and fuse for years,
> even though neither defined a constant in this enum so far.

Perhaps we could get that fixed...?

> Conflicting with FILEID_BCACHEFS_WITH_PARENT is not
> a serious issue, but I encourage Kent to pick different constants
> for bcachefs or keep the bcachefs constants out of this enum.

Happy to do so. Since it seems this enum doesn't have all the constants
I'd need to avoid conflicting with, I might need some help here :)

> It is a slight inconvenience for users that have bcachefs exported
> to NFS clients and upgrade their server, but maybe that is acceptable.
> In overlayfs, we encoded type OVL_FILEID_V0 and switched to encoding
> type OVL_FILEID_V1, but we still accept decoding of both types, neither
> of which are listed in this enum BTW.
> 
> Adding fid types to this enum is not required.
> This enum is a place to standardize and for different fs to share the same
> fid type/encoding as is the case with  FILEID_INO{32,64}_GEN*.
> IMO, the bcachefs constant do not follow the convention in this
> enum and their format is unlikely to be used by other fs, so
> they should not be added to this enum at all.

Eh?

Most of the constants here appear to be completely filesystem specific -
I see UDF, nilfs, btrfs, fat...

And since you also don't want conflicts with fid_types that aren't
defined here, it seems like they really should all be here.