linux-kernel - Re: [PATCH] fs: support filename refcount without atomics

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAGudoHG1VZ8eE_MmD9CPV7TEOg_ozqfHi1r_84Oqf3Ny0XNd9Q@mail.gmail.com>
Date: Fri, 7 Mar 2025 17:32:20 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: brauner@...nel.org, viro@...iv.linux.org.uk, jack@...e.cz, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	io-uring@...r.kernel.org, audit@...r.kernel.org, axboe@...nel.dk
Subject: Re: [PATCH] fs: support filename refcount without atomics

On Fri, Mar 7, 2025 at 5:26 PM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Fri, Mar 07, 2025 at 05:11:55PM +0100, Mateusz Guzik wrote:
> > +++ b/include/linux/fs.h
> > @@ -2765,11 +2765,19 @@ struct audit_names;
> >  struct filename {
> >       const char              *name;  /* pointer to actual string */
> >       const __user char       *uptr;  /* original userland pointer */
> > -     atomic_t                refcnt;
> > +     union {
> > +             atomic_t        refcnt_atomic;
> > +             int             refcnt;
> > +     };
> > +#ifdef CONFIG_DEBUG_VFS
> > +     struct task_struct      *owner;
> > +#endif
> > +     bool                    is_atomic;
> >       struct audit_names      *aname;
> >       const char              iname[];
> >  };
>
> 7 (or 3) byte hole; try to pad.
>
> Would it make more sense to put the bool between aname and iname where
> it will only take one byte instead of 8?

On the stock kernel there is already a 4 byte hole between the
refcount and aname, which is where is_atomic lands with debug
disabled. I.e. no size changes in production kernels with and without
the change.

However, now that you mention it the debug owner field is misplaced --
it should have landed *after* is_atomic. Maybe Christian will be happy
to just move it, otherwise I'm going to include this in a v2.

The iname field is expected to be aligned, so I don't believe
shuffling the is_atomic flag helps anyone:
static_assert(offsetof(struct filename, iname) % sizeof(long) == 0);

-- 
Mateusz Guzik <mjguzik gmail.com>