Re: [PATCH net-next 1/2] xdp: Always use a devmap for XDP_REDIRECT to a device

From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>,
	David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org, Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>
Subject: Re: [PATCH net-next 1/2] xdp: Always use a devmap for XDP_REDIRECT to a device
Date: Mon, 25 Feb 2019 10:47:57 -0800	[thread overview]
Message-ID: <20190225104757.75b622c9@cakuba.netronome.com> (raw)
In-Reply-To: <875ztawvqh.fsf@toke.dk>

On Sat, 23 Feb 2019 13:11:02 +0100, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <brouer@redhat.com> writes:
> > On Fri, 22 Feb 2019 13:37:34 -0800 Jakub Kicinski wrote:
> >> On Fri, 22 Feb 2019 11:13:50 +0100, Toke Høiland-Jørgensen wrote:  
> >> > Jakub Kicinski <jakub.kicinski@netronome.com> writes:    
> >> > > On Thu, 21 Feb 2019 12:56:54 +0100, Toke Høiland-Jørgensen wrote:      
> > [...]  
> >> > >
> >> > > BPF programs don't obey by netns boundaries.  The fact the program is
> >> > > verified in one ns doesn't mean this is the only ns it will be used in :(
> >> > > Meaning if any program is using the redirect map you may need a secret
> >> > > map in every ns.. no?      
> >> > 
> >> > Ah, yes, good point. Totally didn't think about the fact that load and
> >> > attach are decoupled. Hmm, guess I'll just have to move the call to
> >> > alloc_default_map() to the point where the program is attached to an
> >> > interface, then...    
> >> 
> >> Possibly.. and you also need to handle the case where interface with a
> >> program attached is moved, no?  
> 
> Yup, alloc on attach was easy enough; the moving turns out to be the
> tricky part :)
> 
> > True, we need to handle if e.g. a veth gets an XDP program attached and
> > then is moved into a network namespace (as I've already explained to
> > Toke in a meeting).  
> 
> Yeah, I had somehow convinced myself that the XDP program was being
> removed when the interface was being torn down before moving between
> namespaces. Jesper pointed out that this was not in fact the case... :P
> 
> > I'm still not sure how to handle this...  
> 
> There are a couple of options, I think. At least:
> 
> 1. Maintain a flag on struct net_device indicating that this device
>    needs the redirect map allocated, and react to that when interfaces
>    are being moved.
> 
> 2. Lookup the BPF program by ID (which we can get from the driver) on
>    move, and react to the program flag.
> 
> 3. Keep the allocation on program load, but allocate maps for all active
>    namespaces (which would probably need a refcnt mechanism to
>    deallocate things again).
> 
> I think I'm leaning towards #2; possibly combined with a refcnt so we
> can actually deallocate the map in the root namespace when it's not
> needed anymore.

Okay.. what about tail calls?  I think #3 is most reasonable complexity-
-wise, or some mix of #2 and #3 - cnt the programs with legacy
redirects, and then allocate the resources if cnt && name space has any
XDP program attached.

Can users really not be told to just use the correct helper? ;)