linux-kernel - Re: [PATCH 1/2] debugfs: add small file operations for most files

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: <CAMuHMdXbnZc7rYc7ibRNWY6EfRLh-7g0yDeZ3Zk5OXCQ9Cr=cA@mail.gmail.com>
Date: Tue, 26 Nov 2024 09:38:43 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Johannes Berg <johannes@...solutions.net>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Alexander Viro <viro@...iv.linux.org.uk>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Johannes Berg <johannes.berg@...el.com>, 
	linux-m68k <linux-m68k@...ts.linux-m68k.org>
Subject: Re: [PATCH 1/2] debugfs: add small file operations for most files

Hi Johannes,

On Mon, Nov 25, 2024 at 1:37 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> On Tue, Oct 22, 2024 at 3:19 PM Johannes Berg <johannes@...solutions.net> wrote:
> > From: Johannes Berg <johannes.berg@...el.com>
> >
> > As struct file_operations is really big, but (most) debugfs
> > files only use simple_open, read, write and perhaps seek, and
> > don't need anything else, this wastes a lot of space for NULL
> > pointers.
> >
> > Add a struct debugfs_short_fops and some bookkeeping code in
> > debugfs so that users can use that with debugfs_create_file()
> > using _Generic to figure out which function to use.
> >
> > Converting mac80211 to use it where possible saves quite a
> > bit of space:
> >
> > 1010127  205064    1220 1216411  128f9b net/mac80211/mac80211.ko (before)
> >  981199  205064    1220 1187483  121e9b net/mac80211/mac80211.ko (after)
> > -------
> >  -28928 = ~28KiB
> >
> > With a marginal space cost in debugfs:
> >
> >    8701     550      16    9267    2433 fs/debugfs/inode.o (before)
> >   25233     325      32   25590    63f6 fs/debugfs/file.o  (before)
> >    8914     558      16    9488    2510 fs/debugfs/inode.o (after)
> >   25380     325      32   25737    6489 fs/debugfs/file.o  (after)
> > ---------------
> >    +360      +8
> >
> > (All on x86-64)
> >
> > A simple spatch suggests there are more than 300 instances,
> > not even counting the ones hidden in macros like in mac80211,
> > that could be trivially converted, for additional savings of
> > about 240 bytes for each.
> >
> > Signed-off-by: Johannes Berg <johannes.berg@...el.com>
>
> Thanks for your patch, which is now commit 8dc6d81c6b2acc43 ("debugfs:
> add small file operations for most files") upstream.
>
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
>
> > -struct dentry *debugfs_create_file(const char *name, umode_t mode,
> > -                                  struct dentry *parent, void *data,
> > -                                  const struct file_operations *fops)
> > +struct dentry *debugfs_create_file_full(const char *name, umode_t mode,
> > +                                       struct dentry *parent, void *data,
> > +                                       const struct file_operations *fops)
> >  {
> > +       if (WARN_ON((unsigned long)fops &
> > +                   (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT |
> > +                    DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
> > +               return ERR_PTR(-EINVAL);
> >
>
> This warning is triggered during boot on m68k:
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at fs/debugfs/inode.c:457

> > --- a/fs/debugfs/internal.h
> > +++ b/fs/debugfs/internal.h
> > @@ -18,6 +18,7 @@ extern const struct file_operations debugfs_full_proxy_file_operations;
> >
> >  struct debugfs_fsdata {
> >         const struct file_operations *real_fops;
> > +       const struct debugfs_short_fops *short_fops;
> >         union {
> >                 /* automount_fn is used when real_fops is NULL */
> >                 debugfs_automount_t automount;
> > @@ -39,6 +40,11 @@ struct debugfs_fsdata {
> >   * pointer gets its lowest bit set.
> >   */
> >  #define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
> > +/*
> > + * A dentry's ->d_fsdata, when pointing to real fops, is with
> > + * short fops instead of full fops.
> > + */
> > +#define DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT BIT(1)
>
> As the minimum alignment is 2 on m68k, you cannot use bit 1 in pointers
> for your own private use.  See also the comment at
> https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/maple_tree.h#L44

If you want to support truncated debugfs_fops structures, I am
afraid there is no other option than storing a flag, structure type,
or structure size inside the structure. The latter would allow us
to support not just full and short structures, but also arbitrary
truncation, given even more opportunities for size reduction.

E.g. something like

    #define get_method(_ops, _op, _op_fallback) \
            ((_ops->size > offsetof(struct debugfs_fops, _op)) ?
_ops->_op : _op_fallback)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds