Re: [PATCH RFC net-next 3/6] bpf: Allow skb extensions to survive packet scrubbing

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 07/14, Jakub Sitnicki wrote:
> skb_scrub_packet() drops all skb extensions unconditionally via
> skb_ext_reset(). It runs on tunnel encap/decap (ip_tunnel_rcv,
> vxlan_rcv, etc.) and cross-netns forwarding (dev_forward_skb).
> 
> This makes it impossible for a BPF program to pass metadata via
> bpf_skb_ext through a tunnel or across a netns boundary. The extension
> is always lost at the scrub point.
> 
> Introduce skb_ext_scrub() which consults each active extension before
> discarding it. Extensions that request preservation are kept while the
> rest are torn down. When the extension slab is shared with clones, COW
> ensures isolation. Replace the skb_ext_reset() call in
> skb_scrub_packet() with skb_ext_scrub().
> 
> Expose the opt-in mechanism to BPF via the BPF_SKB_EXT_F_NO_SCRUB flag
> for bpf_dynptr_from_skb_ext(). A program that sets this flag when
> creating the extension signals that its metadata should survive
> scrubbing.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  include/linux/bpf.h      |  1 +
>  include/linux/skbuff.h   |  2 ++
>  include/uapi/linux/bpf.h |  3 +-
>  net/core/filter.c        | 11 +++++--
>  net/core/skbuff.c        | 82 ++++++++++++++++++++++++++++++++++++++++++------
>  net/ipv4/udp.c           |  2 +-
>  6 files changed, 86 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6b918a5b61bf..a46ca53c5b27 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -4214,6 +4214,7 @@ static inline int bpf_map_check_op_flags(struct bpf_map *map, u64 flags, u64 all
>  #ifdef CONFIG_BPF_SKB_EXT
>  
>  struct bpf_skb_ext {
> +	u64 flags;
>  	u8 buf[CONFIG_BPF_SKB_EXT_SIZE] __aligned(8);
>  };
>  
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 584d8440d352..66afa5489007 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -5063,6 +5063,7 @@ void *__skb_ext_set(struct sk_buff *skb, enum skb_ext_id id,
>  void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id);
>  void __skb_ext_del(struct sk_buff *skb, enum skb_ext_id id);
>  void __skb_ext_put(struct skb_ext *ext);
> +void skb_ext_scrub(struct sk_buff *skb);
>  
>  static inline void skb_ext_put(struct sk_buff *skb)
>  {
> @@ -5132,6 +5133,7 @@ static inline bool skb_has_extensions(struct sk_buff *skb)
>  static inline void __skb_ext_put(struct skb_ext *ext) {}
>  static inline void skb_ext_put(struct sk_buff *skb) {}
>  static inline void skb_ext_reset(struct sk_buff *skb) {}
> +static inline void skb_ext_scrub(struct sk_buff *skb) {}
>  static inline void skb_ext_del(struct sk_buff *skb, int unused) {}
>  static inline void __skb_ext_copy(struct sk_buff *d, const struct sk_buff *s) {}
>  static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *s) {}
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3eee4467422d..02da170205de 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7734,7 +7734,8 @@ struct bpf_insn_array_value {
>  
>  /* Flags to control bpf_dynptr_from_skb_ext() behavior. */
>  enum {
> -	BPF_SKB_EXT_F_CREATE = (1ULL << 0),
> +	BPF_SKB_EXT_F_CREATE	= (1ULL << 0),
> +	BPF_SKB_EXT_F_NO_SCRUB	= (1ULL << 1),

Do I understand correctly that you do prefer the NO_SCRUB mode? Any reason
we need to have scrub mode? If it's all produced/consumed by bpf, maybe
we can just carry this data unconditionally instead of having a SCRUB/NO_SCRUB
option?