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: <616eab60-0f56-7309-4f0f-c0f96719b688@iogearbox.net>
Date:   Wed, 5 Jan 2022 14:24:16 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yafang Shao <laoar.shao@...il.com>, ast@...nel.org,
        andrii@...nel.org, kafai@...com, songliubraving@...com, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        David Howells <dhowells@...hat.com>, viro@...iv.linux.org.uk
Subject: Re: [PATCH] bpf: allow setting mount device for bpffs

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,
Daniel