On Mon, Apr 22, 2019 at 11:08 AM Saeed Mahameed <saeedm@xxxxxxxxxxxx> wrote:
>
> On Mon, 2019-04-22 at 08:55 -0700, Stanislav Fomichev 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.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > ---
> > include/linux/skbuff.h | 19 +++++++++++--------
> > net/core/flow_dissector.c | 27 +++++++++++++++++----------
> > net/ethernet/eth.c | 5 +++--
> > 3 files changed, 31 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index beaedd487be1..ee7d1a85f624 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -1284,7 +1284,8 @@ bool __skb_flow_bpf_dissect(struct bpf_prog
> > *prog,
> > const struct sk_buff *skb,
> > struct flow_dissector *flow_dissector,
> > struct bpf_flow_keys *flow_keys);
> > -bool __skb_flow_dissect(const struct sk_buff *skb,
> > +bool __skb_flow_dissect(const struct net *net,
> > + const struct sk_buff *skb,
> > struct flow_dissector *flow_dissector,
> > void *target_container,
> > void *data, __be16 proto, int nhoff, int hlen,
> > @@ -1294,8 +1295,8 @@ static inline bool skb_flow_dissect(const
> > struct sk_buff *skb,
> > struct flow_dissector
> > *flow_dissector,
> > void *target_container, unsigned
> > int flags)
> > {
> > - return __skb_flow_dissect(skb, flow_dissector,
> > target_container,
> > - NULL, 0, 0, 0, flags);
> > + return __skb_flow_dissect(NULL, skb, flow_dissector,
> > + target_container, NULL, 0, 0, 0,
> > flags);
> > }
> >
> > static inline bool skb_flow_dissect_flow_keys(const struct sk_buff
> > *skb,
> > @@ -1303,18 +1304,19 @@ static inline bool
> > skb_flow_dissect_flow_keys(const struct sk_buff *skb,
> > unsigned int flags)
> > {
> > memset(flow, 0, sizeof(*flow));
> > - return __skb_flow_dissect(skb, &flow_keys_dissector, flow,
> > - NULL, 0, 0, 0, flags);
> > + return __skb_flow_dissect(NULL, skb, &flow_keys_dissector,
> > + flow, NULL, 0, 0, 0, flags);
> > }
> >
> > static inline bool
> > -skb_flow_dissect_flow_keys_basic(const struct sk_buff *skb,
> > +skb_flow_dissect_flow_keys_basic(const struct net *net,
> > + const struct sk_buff *skb,
> > struct flow_keys_basic *flow, void
> > *data,
> > __be16 proto, int nhoff, int hlen,
> > unsigned int flags)
> > {
> > memset(flow, 0, sizeof(*flow));
> > - return __skb_flow_dissect(skb, &flow_keys_basic_dissector,
> > flow,
> > + return __skb_flow_dissect(net, skb, &flow_keys_basic_dissector,
> > flow,
> > data, proto, nhoff, hlen, flags);
> > }
> >
> > @@ -2494,7 +2496,8 @@ static inline void
> > skb_probe_transport_header(struct sk_buff *skb)
> > if (skb_transport_header_was_set(skb))
> > return;
> >
> > - if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0,
> > 0))
> > + if (skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
> > + NULL, 0, 0, 0, 0))
> > skb_set_transport_header(skb, keys.control.thoff);
> > }
> >
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index ef6d9443cc75..f32c7e737fc6 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -722,6 +722,7 @@ bool bpf_flow_dissect(struct bpf_prog *prog,
> > struct bpf_flow_dissector *ctx,
> >
> > /**
> > * __skb_flow_dissect - extract the flow_keys struct and return it
> > + * @net: associated network namespace, derived from @skb if NULL
> > * @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
> > @@ -738,7 +739,8 @@ bool bpf_flow_dissect(struct bpf_prog *prog,
> > struct bpf_flow_dissector *ctx,
> > *
> > * Caller must take care of zeroing target container memory.
> > */
> > -bool __skb_flow_dissect(const struct sk_buff *skb,
> > +bool __skb_flow_dissect(const struct net *net,
> > + const struct sk_buff *skb,
> > struct flow_dissector *flow_dissector,
> > void *target_container,
> > void *data, __be16 proto, int nhoff, int hlen,
>
> LGTM,
>
> Reviewed-by: Saeed Mahameed <saeedm@xxxxxxxxxxxx>
>
> Side Note: it is not this patch's fault, but I think that there is lots
> of room for improvement in __skb_flow_dissect parameters set, it seems
> that the function can receives two separate sets of parameters and each
> set is only used when a valid skb is passed, while the other set is
> used when skb is null but data ptr isn't, this makes this function very
> hard to read and track it's flow.
>
> My 2 cent is that we need two versions of __skb_flow_dissect, one that
> uses skb and the other uses data pointer, and the skb version can
> perfectly use the "data" ptr version, it could be a hard re-factoring
> task, but i think it is doable.
Agreed, the only problem with separating those two is __skb_header_pointer
which is occasionally called to fetch more data from the skb frags (for
both data!=NULL and skb!=NULL cases).
I was thinking about changing the order of input params to move skb
to the end (so it looks more optional) and to move all this !data initialization
into a wrapper.
I might look into this at some point..
