netdev - Re: [PATCH net-next 02/13] sk_buff: add skb extension infrastructure

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-next>] [day] [month] [year] [list]
Date:   Tue, 11 Dec 2018 16:07:06 -0800 (PST)
From:   Mat Martineau <mathew.j.martineau@...ux.intel.com>
To:     Florian Westphal <fw@...len.de>
cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 02/13] sk_buff: add skb extension
 infrastructure


On Mon, 11 Dec 2018, Florian Westphal wrote:

...

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index b1831a5ca173..d715736eb734 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h

...

> @@ -3896,6 +3906,113 @@ static inline void nf_conntrack_get(struct nf_conntrack *nfct)
>                 atomic_inc(&nfct->use);
>  }
>  #endif
> +
> +#ifdef CONFIG_SKB_EXTENSIONS
> +enum skb_ext_id {
> +#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> +       SKB_EXT_BRIDGE_NF,
> +#endif
> +       SKB_EXT_NUM, /* must be last */
> +};

Could enum skb_ext_id always be defined, with none of the SKB_EXT_* values 
conditionally excluded? In combination with some alternate function 
definitions below (see later comments) I think this could reduce the need 
for CONFIG_SKB_EXTENSIONS preprocessor conditionals throughout the net 
code.

> +
> +/**
> + *     struct skb_ext - sk_buff extensions
> + *     @refcnt: 1 on allocation, deallocated on 0
> + *     @offset: offset to add to @data to obtain extension address
> + *     @chunks: size currently allocated, stored in SKB_EXT_ALIGN_SHIFT units
> + *     @data: start of extension data, variable sized
> + *
> + *     Note: offsets/lengths are stored in chunks of 8 bytes, this allows
> + *     to use 'u8' types while allowing up to 2kb worth of extension data.
> + */
> +struct skb_ext {
> +       refcount_t refcnt;
> +       u8 offset[SKB_EXT_NUM]; /* in chunks of 8 bytes */
> +       u8 chunks;              /* same */
> +       char data[0] __aligned(8);
> +};
> +
> +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_free(struct skb_ext *ext);
> +
> +static inline void __skb_ext_put(struct skb_ext *ext)
> +{
> +       if (ext && refcount_dec_and_test(&ext->refcnt))
> +               __skb_ext_free(ext);
> +}
> +
> +static inline void skb_ext_put(struct sk_buff *skb)
> +{
> +       if (skb->active_extensions)
> +               __skb_ext_put(skb->extensions);
> +}
> +
> +static inline void skb_ext_get(struct sk_buff *skb)
> +{
> +       if (skb->active_extensions) {
> +               struct skb_ext *ext = skb->extensions;
> +
> +               if (ext)
> +                       refcount_inc(&ext->refcnt);
> +       }
> +}
> +
> +static inline void __skb_ext_copy(struct sk_buff *dst,
> +                                 const struct sk_buff *src)
> +{
> +       dst->active_extensions = src->active_extensions;
> +
> +       if (src->active_extensions) {
> +               struct skb_ext *ext = src->extensions;
> +
> +               if (ext)
> +                       refcount_inc(&ext->refcnt);
> +               dst->extensions = ext;
> +       }
> +}
> +
> +static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *src)
> +{
> +       skb_ext_put(dst);
> +       __skb_ext_copy(dst, src);
> +}
> +
> +static inline bool __skb_ext_exist(const struct skb_ext *ext, enum skb_ext_id i)
> +{
> +       return !!ext->offset[i];
> +}
> +
> +static inline bool skb_ext_exist(const struct sk_buff *skb, enum skb_ext_id id)
> +{
> +       return skb->active_extensions & (1 << id);
> +}
> +
> +static inline void skb_ext_del(struct sk_buff *skb, enum skb_ext_id id)
> +{
> +       if (skb_ext_exist(skb, id))
> +               __skb_ext_del(skb, id);
> +}
> +
> +static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id)
> +{
> +       if (skb_ext_exist(skb, id)) {
> +               struct skb_ext *ext = skb->extensions;
> +
> +               if (ext && __skb_ext_exist(ext, id))
> +                       return (void *)ext + (ext->offset[id] << 3);
> +       }
> +
> +       return NULL;
> +}
> +#else
> +static inline void skb_ext_put(struct sk_buff *skb) {}
> +static inline void skb_ext_get(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) {}

For the !CONFIG_SKB_EXTENSIONS case, an alternate definition of 
skb_ext_exist() that always returns false would be useful to reduce the 
need for preprocessor conditionals. A similar skb_ext_find() that always 
returns NULL might also be helpful.

> +#endif /* CONFIG_SKB_EXTENSIONS */
> +
>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>  static inline void nf_bridge_put(struct nf_bridge_info *nf_bridge)
>  {

Thanks,

--
Mat Martineau
Intel OTC