Message-ID: <CAGudoHFQ9TtG-5__38-ND4KTxYCpEKVv_X9HhZixcdnVMUBEwQ@mail.gmail.com>
Date: Mon, 29 Jul 2024 14:12:15 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Aleksa Sarai <cyphar@...har.com>
Cc: Florian Weimer <fweimer@...hat.com>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org, 
	Dave Chinner <dchinner@...hat.com>, Christian Brauner <brauner@...nel.org>
Subject: Re: Testing if two open descriptors refer to the same inode

On Mon, Jul 29, 2024 at 1:47 PM Aleksa Sarai <cyphar@...har.com> wrote:
>
> On 2024-07-29, Mateusz Guzik <mjguzik@...il.com> wrote:
> > On Mon, Jul 29, 2024 at 12:57 PM Florian Weimer <fweimer@...hat.com> wrote:
> > >
> > > * Mateusz Guzik:
> > >
> > > > On Mon, Jul 29, 2024 at 12:40:35PM +0200, Florian Weimer wrote:
> > > >> * Mateusz Guzik:
> > > >>
> > > >> > On Mon, Jul 29, 2024 at 08:55:46AM +0200, Florian Weimer wrote:
> > > >> >> It was pointed out to me that inode numbers on Linux are no longer
> > > >> >> expected to be unique per file system, even for local file systems.
> > > >> >
> > > >> > I don't know if I'm parsing this correctly.
> > > >> >
> > > >> > Are you claiming on-disk inode numbers are not guaranteed unique per
> > > >> > filesystem? It sounds like utter breakage, with capital 'f'.
> > > >>
> > > >> Yes, POSIX semantics and traditional Linux semantics for POSIX-like
> > > >> local file systems are different.
> > > >
> > > > Can you link me some threads about this?
> > >
> > > Sorry, it was an internal thread.  It's supposed to be common knowledge
> > > among Linux file system developers.  Aleksa referenced LSF/MM
> > > discussions.
> > >
> >
> > So much for open development :-P
> >
> > > > I had this in mind (untested modulo compilation):
> > > >
> > > > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > > > index 300e5d9ad913..5723c3e82eac 100644
> > > > --- a/fs/fcntl.c
> > > > +++ b/fs/fcntl.c
> > > > @@ -343,6 +343,13 @@ static long f_dupfd_query(int fd, struct file *filp)
> > > >       return f.file == filp;
> > > >  }
> > > >
> > > > +static long f_dupfd_query_inode(int fd, struct file *filp)
> > > > +{
> > > > +     CLASS(fd_raw, f)(fd);
> > > > +
> > > > +     return f.file->f_inode == filp->f_inode;
> > > > +}
> > > > +
> > > >  static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > > >               struct file *filp)
> > > >  {
> > > > @@ -361,6 +368,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> > > >       case F_DUPFD_QUERY:
> > > >               err = f_dupfd_query(argi, filp);
> > > >               break;
> > > > +     case F_DUPFD_QUERY_INODE:
> > > > +             err = f_dupfd_query_inode(argi, filp);
> > > > +             break;
> > > >       case F_GETFD:
> > > >               err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
> > > >               break;
> > > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > > index c0bcc185fa48..2e93dbdd8fd2 100644
> > > > --- a/include/uapi/linux/fcntl.h
> > > > +++ b/include/uapi/linux/fcntl.h
> > > > @@ -16,6 +16,8 @@
> > > >
> > > >  #define F_DUPFD_QUERY        (F_LINUX_SPECIFIC_BASE + 3)
> > > >
> > > > +#define F_DUPFD_QUERY_INODE (F_LINUX_SPECIFIC_BASE + 4)
> > > > +
> > > >  /*
> > > >   * Cancel a blocking posix lock; internal use only until we expose an
> > > >   * asynchronous lock api to userspace:
> > >
> > > It's certainly much easier to use than name_to_handle_at, so it looks
> > > like a useful option to have.
> > >
> > > Could we return a three-way comparison result for sorting?  Or would
> > > that expose too much about kernel pointer values?
> > >
> >
> > As is this would sort by inode *address* which I don't believe is of
> > any use -- the order has to be assumed arbitrary.
> >
> > Perhaps there is something which is reliably the same and can be
> > combined with something else to be unique system-wide (the magic
> > handle thing?).
> >
> > But even then you would need to justify trying to sort by fcntl calls,
> > which sounds pretty dodgey to me.
>
> Programs need to key things by (dev, ino) currently, so you need to be
> able to get some kind of ordinal that you can sort with.
>

That I know, except normally that's done by sorting by (f)stat results.

> If we really want to make an interface to let you do this without
> exposing hashes in statx, then kcmp(2) makes more sense, but having to
> keep a file descriptor for each entry in a hashtable would obviously
> cause -EMFILE issues.
>

Agreed, hence the proposal to extend statx.

> > Given that thing I *suspect* statx() may want to get extended with
> > some guaranteed unique identifier. Then you can sort in userspace all
> > you want.
>
> Yeah, this is what the hashed fhandle patch I have does.
>

Ok, I see your other e-mail.

> > Based on your opening mail I assumed you only need to check 2 files,
> > for which the proposed fcntl does the trick.
> >
> > Or to put it differently: there seems to be more to the picture than
> > in the opening mail, so perhaps you could outline what you are looking
> > for.
>
> Hardlink detection requires creating a hashmap of (dev, ino) to find
> hardlinks. Pair-wise checking is not sufficient for that usecase (which
> AFAIK is the most common thing people use inode numbers for -- it's at
> least probably the most common thing people run in practice since
> archive tools do this.)
>

So if you have *numerous* files to check then indeed the fcntl is no
good, but the sorting variant is no good either -- you don't know what
key to look stuff up by since you don't know any of the addresses
(obfuscated or otherwise).

There needs to be a dev + ino replacement which retains the property
of being reliable between reboots and so on.

Since you said you have a patchset which exports something in statx,
chances are this is sorted out -- I'm gong to wait for that, meanwhile
I'm not going to submit my fcntl anywhere -- hopefuly it will be
avoided. :)
--
Mateusz Guzik <mjguzik gmail.com>

Powered by blists - more mailing lists