netdev - Re: [PATCH bpf-next v6 3/9] net: plumb network namespace into __skb_flow

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]
Date:   Mon, 22 Apr 2019 18:08:48 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "sdf@...gle.com" <sdf@...gle.com>
CC:     "daniel@...earbox.net" <daniel@...earbox.net>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "peterpenkov96@...il.com" <peterpenkov96@...il.com>,
        "willemb@...gle.com" <willemb@...gle.com>,
        "simon.horman@...ronome.com" <simon.horman@...ronome.com>,
        "ast@...nel.org" <ast@...nel.org>
Subject: Re: [PATCH bpf-next v6 3/9] net: plumb network namespace into
 __skb_flow_dissect

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@...gle.com>
> ---
>  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@...lanox.com>

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.