On Wed, Jan 5, 2022 at 9:24 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > Cc: David Howells <dhowells@xxxxxxxxxx>
> > ---
> > 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
