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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 12 Dec 2018 10:45:42 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 02/13] sk_buff: add skb extension infrastructure

On Wed, Dec 12, 2018 at 10:40 AM Florian Westphal <fw@...len.de> wrote:
>
> Willem de Bruijn <willemdebruijn.kernel@...il.com> wrote:
> > > +#ifdef CONFIG_SKB_EXTENSIONS
> > > +       __u8                    active_extensions;
> > > +#endif
> >
> > This byte could be saved by moving the bits to the first byte of the
> > new extension.
>
> I tried to do this, but could not resolve following problem:
> - extensions a and b are active
> - skb is cloned
> - extension a is to be disabled
>
> In this patch series, we can just clear the 'a' bit from
> from clone->active_extensions.
>
> But if its part of the extension itself, we must first need to
> be able to duplicate the extension blob before we can disable
> the extension, which means that attempt to disable an extension
> would now fail on -ENOMEM.  I think disabling should always work.

Absolutely. I certainly overlooked that complicating factor.

> (its not a problem at the moment for either secpath or bridge
>  as both use distinct pointers in sk_buff).
>
> > The likely cold cacheline now needs to be checked, but
> > only if the pointer is non-NULL.
>
> The upside of the 'active_extension' member is that we can keep the
> pointer in uninitialized state for the active_extensions == 0 case,
> i.e., when allocating a new skb skb->extensions is not initialized.
>
> > If exclusively using accessor
> > functions to access the struct, even this can be avoided if encoding
> > the first 3 or 7 active extensions in the pointer itself.
>
> Yes, that would work, but requires to init the pointer on skb allocation
> plus a few tricks to align the allocation so we have three bits to use
> on arches where kmalloc doesn't guarantee 8-byte-aligned pointers.
>
> Would also need a 'plan b' in place when extension number 4 arrives.

My assumed fallback was the first byte(s) of the extension, but as you
point out above, we cannot rely on that.

Okay, in that case the current solution is definitely preferable.

Ack also on the zeroing and alignment. Thanks for pointing out the
various pros and cons.