Re: [RFC bpf-next 3/7] net: plumb network namespace into __skb_flow_dissect

From: Stanislav Fomichev <sdf@fomichev.me>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Stanislav Fomichev <sdf@google.com>,
	Network Development <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	simon.horman@netronome.com, Willem de Bruijn <willemb@google.com>
Subject: Re: [RFC bpf-next 3/7] net: plumb network namespace into __skb_flow_dissect
Date: Tue, 5 Feb 2019 12:40:42 -0800	[thread overview]
Message-ID: <20190205204042.GC10769@mini-arch> (raw)
In-Reply-To: <CAF=yD-JOpmA-ahiyH07kpNXmUO7z-V3oqWRQ6D5S+R1TSgwFPA@mail.gmail.com>

On 02/05, Willem de Bruijn wrote:
> On Tue, Feb 5, 2019 at 12:57 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > This new argument will be used in the next patches for the
> > eth_get_headlen use case. eth_get_headlen calls flow dissector
> > with only data (without skb) so there is currently no way to
> > pull attached BPF flow dissector program. With this new argument,
> > we can amend the callers to explicitly pass network namespace
> > so we can use attached BPF program.
> >
> > Note: WARN_ON_ONCE(!net) will now trigger for eth_get_headlen users.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> 
> >  /**
> >   * __skb_flow_dissect - extract the flow_keys struct and return it
> > + * @net: associated network namespace, if NULL pulled from skb
> >   * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
> >   * @flow_dissector: list of keys to dissect
> >   * @target_container: target structure to put dissected values into
> > @@ -739,7 +740,8 @@ bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
> >   *
> >   * Caller must take care of zeroing target container memory.
> >   */
> > -bool __skb_flow_dissect(const struct sk_buff *skb,
> > +bool __skb_flow_dissect(struct net *net,
> > +                       const struct sk_buff *skb,
> >                         struct flow_dissector *flow_dissector,
> >                         void *target_container,
> >                         void *data, __be16 proto, int nhoff, int hlen,
> > @@ -799,12 +801,11 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >
> >                 rcu_read_lock();
> >
> > -               if (skb->dev)
> > -                       attached = rcu_dereference(dev_net(skb->dev)->flow_dissector_prog);
> > -               else if (skb->sk)
> > -                       attached = rcu_dereference(sock_net(skb->sk)->flow_dissector_prog);
> > -               else
> > -                       WARN_ON_ONCE(1);
> > +               if (!net && skb)
> > +                       net = skb_net(skb);
> > +               if (net)
> > +                       attached = rcu_dereference(net->flow_dissector_prog);
> > +               WARN_ON_ONCE(!net);
> 
> Instead of this just call skb_net(skb) in all callers of
> __skb_flow_dissect that are called with an skb argument directly?
> 
> It may have to be able to handle skb == NULL args.
Ack, will look into it.