netdev - Re: [PATCH] bpf: allow setting mount device for bpffs

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: <CALOAHbBi4HYUd+AD+F8DrCUPrh8-E3HJC=RPMTw3dNLKHAHczg@mail.gmail.com>
Date:   Fri, 7 Jan 2022 13:48:30 +0800
From:   Yafang Shao <laoar.shao@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, netdev <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, David Howells <dhowells@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] bpf: allow setting mount device for bpffs

On Wed, Jan 5, 2022 at 9:24 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 12/26/21 5:56 PM, Yafang Shao wrote:
> > We noticed our tc ebpf tools can't start after we upgrade our in-house
> > kernel version from 4.19 to 5.10. That is because of the behaviour change
> > in bpffs caused by commit
> > d2935de7e4fd ("vfs: Convert bpf to use the new mount API").
> >
> > In our tc ebpf tools, we do strict environment check. If the enrioment is
> > not match, we won't allow to start the ebpf progs. One of the check is
> > whether bpffs is properly mounted. The mount information of bpffs in
> > kernel-4.19 and kernel-5.10 are as follows,
> >
> > - kenrel 4.19
> > $ mount -t bpf bpffs /sys/fs/bpf
> > $ mount -t bpf
> > bpffs on /sys/fs/bpf type bpf (rw,relatime)
> >
> > - kernel 5.10
> > $ mount -t bpf bpffs /sys/fs/bpf
> > $ mount -t bpf
> > none on /sys/fs/bpf type bpf (rw,relatime)
> >
> > The device name in kernel-5.10 is displayed as none instead of bpffs,
> > then our environment check fails. Currently we modify the tools to adopt to
> > the kernel behaviour change, but I think we'd better change the kernel code
> > to keep the behavior consistent.
> >
> > After this change, the mount information will be displayed the same with
> > the behavior in kernel-4.19, for example,
> >
> > $ mount -t bpf bpffs /sys/fs/bpf
> > $ mount -t bpf
> > bpffs on /sys/fs/bpf type bpf (rw,relatime)
> >
> > Fixes: d2935de7e4fd ("vfs: Convert bpf to use the new mount API")
> > Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> > Cc: David Howells <dhowells@...hat.com>
> > ---
> >   kernel/bpf/inode.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> > index 80da1db47c68..5a8b729afa91 100644
> > --- a/kernel/bpf/inode.c
> > +++ b/kernel/bpf/inode.c
> > @@ -648,12 +648,26 @@ static int bpf_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >       int opt;
> >
> >       opt = fs_parse(fc, bpf_fs_parameters, param, &result);
> > -     if (opt < 0)
> > +     if (opt < 0) {
> >               /* We might like to report bad mount options here, but
> >                * traditionally we've ignored all mount options, so we'd
> >                * better continue to ignore non-existing options for bpf.
> >                */
> > -             return opt == -ENOPARAM ? 0 : opt;
> > +             if (opt == -ENOPARAM) {
> > +                     if (strcmp(param->key, "source") == 0) {
> > +                             if (param->type != fs_value_is_string)
> > +                                     return 0;
> > +                             if (fc->source)
> > +                                     return 0;
> > +                             fc->source = param->string;
> > +                             param->string = NULL;
> > +                     }
> > +
> > +                     return 0;
> > +             }
> > +
> > +             return opt;
> > +     }
>
> I don't think we need to open code this? Couldn't we just do something like:
>
>          [...]
>
>          opt = fs_parse(fc, bpf_fs_parameters, param, &result);
>          if (opt == -ENOPARAM) {
>                  opt = vfs_parse_fs_param_source(fc, param);
>                  if (opt != -ENOPARAM)
>                          return opt;
>                  return 0;
>          }
>          if (opt < 0)
>                  return opt;
>
>          [...]
>
> See also 0858d7da8a09 ("ramfs: fix mount source show for ramfs") where they
> had a similar issue.
>

Thanks for the suggestion. I will update it.

nit:  vfs_parse_fs_param_source() is introduced in commit d1d488d81370
("fs: add vfs_parse_fs_param_source() helper"), so the updated one
can't be directly backported to 5.10.

-- 
Thanks
Yafang