netdev - Re: [PATCH nf-next] netfilter: nf

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: <87o7w04jjb.fsf@toke.dk>
Date:   Wed, 31 Aug 2022 15:41:44 +0200
From:   Toke Høiland-Jørgensen <toke@...nel.org>
To:     Florian Westphal <fw@...len.de>
Cc:     netfilter-devel@...r.kernel.org, bpf@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH nf-next] netfilter: nf_tables: add ebpf expression

Florian Westphal <fw@...len.de> writes:

> Toke Høiland-Jørgensen <toke@...nel.org> wrote:
>> > Tag and program id are dumped to userspace on 'list' to allow to see which
>> > program is in use in case the filename isn't available/present.
>> 
>> It seems a bit odd to include the file path in the kernel as well.
>
> Its needed to be able to re-load the ruleset.

How does that work, exactly? Is this so that the userspace binary can
query the current ruleset, and feed it back to the kernel expecting it
to stay the same? Because in that case, if the pinned object goes away
in the meantime (or changes to a different program), this could lead to
some really hard to debug errors, where a reload subtly changes the
behaviour because the BPF program is not in fact the same.

Using IDs would avoid this ambiguity at least, so I think that's a
better solution. We'd have to make sure the BPF program is not released
completely until after the reload has finished, so that it doesn't
suddenly disappear.

>> But doesn't NFT already have a per-rule comment feature,
>> so why add another specifically for BPF?
>
> You can attach up to 256 bytes to a rule, yes.
> Might not be enough for a longer path, and there could be multiple
> expressions in the same rule.
>
> This way was the most simple solution.

My point here was more that if it's just a label for human consumption,
the comment field should be fine, didn't realise it was needed for the
tool operation (and see above re: that).

>> Instead we could just teach the
>> userspace utility to extract metadata from the BPF program (based on the
>> ID) like bpftool does. This would include the program name, BTW, so it
>> does have a semantic identifier.
>
> Sure, I could change the grammar so it expects a tag or ID, e.g.
> 'ebpf id 42'
>
> If thats preferred, I can change this, it avoids the need for storing
> the name.

I think for echoing back, just relying on the ID is better as that is at
least guaranteed to stay constant for the lifetime of the BPF program in
the kernel. We could still support the 'pinned <path>' syntax on the
command line so that the initial load could be done from a pinned file,
just as a user interface improvement...

>> > cbpf bytecode isn't supported.
>> > add rule ... ebpf pinned "/sys/fs/bpf/myprog"
>> 
>> Any plan to also teach the nft binary to load a BPF program from an ELF
>> file (instead of relying on pinning)?
>
> I used pinning because that is what '-m bpf' uses.

I'm not against supporting pinning, per se (except for the issues noted
above), but we could do multiple things, including supporting loading
the program from an object file. This is similar to how TC operates, for
instance...

-Toke