On Sun, Jun 30, 2019 at 9:57 PM Willem de Bruijn
<willemdebruijn.ker...@gmail.com> wrote:
>
> On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <a...@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 <herb...@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 <a...@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);
> >
