Re: [PATCH net-next 1/3] macsec: remove superfluous function calls

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Sun, Jun 30, 2019 at 9:57 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@domdv.de> wrote:
> >
> > Remove superfluous skb_share_check() and skb_unshare().
> > macsec_decrypt is only called by macsec_handle_frame which
> > already does a skb_unshare().
>
> There is a subtle difference. skb_unshare() acts on cloned skbs, not
> shared skbs.
>
> It creates a private copy of data if clones may access it
> concurrently, which clearly is needed when modifying for decryption.
>
> At rx_handler, I don't think a shared skb happen (unlike clones, e.g.,
> from packet sockets). But it is peculiar that most, if not all,
> rx_handlers seem to test for it. That have started with the bridge
> device:
>
> commit 7b995651e373d6424f81db23f2ec503306dfd7f0
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Sun Oct 14 00:39:01 2007 -0700
>
>     [BRIDGE]: Unshare skb upon entry
>
>     Due to the special location of the bridging hook, it should never see a
>     shared packet anyway (certainly not with any in-kernel code).  So it
>     makes sense to unshare the skb there if necessary as that will greatly
>     simplify the code below it (in particular, netfilter).
>
> Anyway, remove the check only if certain.

Did you see this point? I notice the v2 without this mentioned. I
don't think you can remove an skb_share_check solely on the basis of a
previous skb_unshare. I agree that here a shared skb is unlikely, but
that is for a very different, less obvious, reason.




>
> >
> > Signed-off-by: Andreas Steinmetz <ast@domdv.de>
> >
> > --- a/drivers/net/macsec.c      2019-06-30 22:02:54.906908179 +0200
> > +++ b/drivers/net/macsec.c      2019-06-30 22:03:07.315785186 +0200
> > @@ -939,9 +939,6 @@
> >         u16 icv_len = secy->icv_len;
> >
> >         macsec_skb_cb(skb)->valid = false;
> > -       skb = skb_share_check(skb, GFP_ATOMIC);
> > -       if (!skb)
> > -               return ERR_PTR(-ENOMEM);
> >
> >         ret = skb_cow_data(skb, 0, &trailer);
> >         if (unlikely(ret < 0)) {
> > @@ -973,11 +970,6 @@
> >
> >                 aead_request_set_crypt(req, sg, sg, len, iv);
> >                 aead_request_set_ad(req, macsec_hdr_len(macsec_skb_cb(skb)->has_sci));
> > -               skb = skb_unshare(skb, GFP_ATOMIC);
> > -               if (!skb) {
> > -                       aead_request_free(req);
> > -                       return ERR_PTR(-ENOMEM);
> > -               }
> >         } else {
> >                 /* integrity only: all headers + data authenticated */
> >                 aead_request_set_crypt(req, sg, sg, icv_len, iv);
> >