Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 15:21:41 +0200
From:   Florian Westphal <fw@strlen.de>
To:     Toke =?iso-8859-15?Q?H=F8iland-J=F8rgensen?= <toke@redhat.com>
Cc:     Florian Westphal <fw@strlen.de>, Daniel Xu <dxu@dxuuu.xyz>,
        bpf@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        coreteam@netfilter.org, netfilter-devel@vger.kernel.org,
        daniel@iogearbox.net, dsahern@kernel.org
Subject: Re: [PATCH bpf-next 0/7] Support defragmenting IPv(4|6) packets in
 BPF
Message-ID: <20230629132141.GA10165@breakpoint.cc>
References: <cover.1687819413.git.dxu@dxuuu.xyz>
 <874jmthtiu.fsf@toke.dk>
 <20230627154439.GA18285@breakpoint.cc>
 <87o7kyfoqf.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87o7kyfoqf.fsf@toke.dk>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Toke H?iland-J?rgensen <toke@redhat.com> wrote:
> Florian Westphal <fw@strlen.de> writes:
> > For bpf a flag during link attachment seemed like the best way
> > to go.
> 
> Right, I wasn't disputing that having a flag to load a module was a good
> idea. On the contrary, I was thinking we'd need many more of these
> if/when BPF wants to take advantage of more netfilter code. Say, if a
> BPF module wants to call into TPROXY, that module would also need go be
> loaded and kept around, no?

That seems to be a different topic that has nothing to do with
either bpf_link or netfilter?

If the program calls into say, TPROXY, then I'd expect that this needs
to be handled via kfuncs, no? Or if I misunderstand, what do you mean
by "call into TPROXY"?

And if so, thats already handled at bpf_prog load time, not
at link creation time, or do I miss something here?

AFAIU, if prog uses such kfuncs, verifier will grab needed module ref
and if module isn't loaded the kfuncs won't be found and program load
fails.

> I was thinking something along the lines of just having a field
> 'netfilter_modules[]' where userspace could put an arbitrary number of
> module names into, and we'd load all of them and put a ref into the
> bpf_link.

Why?  I fail to understand the connection between bpf_link, netfilter
and modules.  What makes netfilter so special that we need such a
module array, and what does that have to do with bpf_link interface?

> In principle, we could just have that be a string array f
> module names, but that's probably a bit cumbersome (and, well, building
> a generic module loader interface into the bpf_like API is not
> desirable either). But maybe with an explicit ENUM?

What functionality does that provide? I can't think of a single module
where this functionality is needed.

Either we're talking about future kfuncs, then, as far as i understand
how kfuncs work, this is handled at bpf_prog load time, not when the
bpf_link is created.

Or we are talking about implicit dependencies, where program doesn't
call function X but needs functionality handled earlier in the pipeline?

The only two instances I know where this is the case for netfilter
is defrag + conntrack.

> > For conntrack, we MIGHT be able to not need a flag but
> > maybe verifier could "guess" based on kfuncs used.
> 
> If the verifier can just identify the modules from the kfuncs and do the
> whole thing automatically, that would of course be even better from an
> ease-of-use PoV. Not sure what that would take, though? I seem to recall
> having discussions around these lines before that fell down on various
> points.

AFAICS the conntrack kfuncs are wired to nf_conntrack already, so I
would expect that the module has to be loaded already for the verifier
to accept the program.

Those kfuncs are not yet exposed to NETFILTER program types.
Once they are, all that would be needed is for the netfilter bpf_link
to be able tp detect that the prog is calling into those kfuncs, and
then make the needed register/unregister calls to enable the conntrack
hooks.

Wheter thats better than using an explicit "please turn on conntrack for
me", I don't know.  Perhaps future bpf programs could access skb->_nfct
directly without kfuncs so I'd say the flag is a better approach
from an uapi point of view.