Re: [PATCH net] Revert "net: macsec: use skb_ensure_writable_head_tail to expand the skb"

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Tue, 16 Jan, 2024 12:45:46 -0800 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote:
> On Tue, 16 Jan, 2024 14:51:19 +0100 Sabrina Dubroca <sd@queasysnail.net> wrote:
>> 2024-01-16, 11:39:35 +0100, Paolo Abeni wrote:
>>> On Sun, 2024-01-14 at 09:42 -0800, Rahul Rameshbabu wrote:
>>> > This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5.
>>> > 
>>> > Using skb_ensure_writable_head_tail without a call to skb_unshare causes
>>> > the MACsec stack to operate on the original skb rather than a copy in the
>>> > macsec_encrypt path. This causes the buffer to be exceeded in space, and
>>> > leads to warnings generated by skb_put operations. 
>>> 
>>> This part of the changelog is confusing to me. It looks like the skb
>>> should be uncloned under the same conditions before and after this
>>> patch (and/or the reverted)??!
>>
>> I don't think so. The old code was doing unshare +
>> expand. skb_ensure_writable_head_tail calls pskb_expand_head without
>> unshare, which doesn't give us a fresh sk_buff, only takes care of the
>> headroom/tailroom. Or do I need more coffee? :/
>
> Sabrina's analysis is correct. We no longer get a fresh sk_buff with
> this commit.
>
>>
>>> Possibly dev->needed_headroom/needed_tailroom values are incorrect?!?
>>
>> That's also possible following commit a73d8779d61a ("net: macsec:
>> introduce mdo_insert_tx_tag"). Then this revert would only be hiding
>> the issue.
>
> Ah, I think that is an interesting point.
>
>     static void macsec_set_head_tail_room(struct net_device *dev)
>     {
>     	struct macsec_dev *macsec = macsec_priv(dev);
>     	struct net_device *real_dev = macsec->real_dev;
>     	int needed_headroom, needed_tailroom;
>     	const struct macsec_ops *ops;
>
>     	ops = macsec_get_ops(macsec, NULL);
>     	if (ops) {
>
> This condition should really be ops && ops->mdo_insert_tx_tags. Let me
> retest with this change and post back. That said, I am wondering if we
> still need a fresh skb in the macsec stack or not as was done previously
> with skb_unshare/skb_copy_expand or not.

Both fixing the headroom/tailroom management in this commit,
a73d8779d61a ("net: macsec: introduce mdo_insert_tx_tag"), as well as
simply reverting this commit does not resolve the issue. I also end up
needing to revert b34ab3527b96 ("net: macsec: use
skb_ensure_writable_head_tail to expand the skb"), so that a fresh
sk_buff is created to avoid the panic mentioned in this commit.

I think we can do one of two things.

1. We merge this patch, and I send a follow-up fix with regards to the
   issues in b34ab3527b96.
2. I send a v2 where I add an additional patch for fixing the issues in
   b34ab3527b96.

>
>     		needed_headroom = ops->needed_headroom;
>     		needed_tailroom = ops->needed_tailroom;
>     	} else {
>     		needed_headroom = MACSEC_NEEDED_HEADROOM;
>     		needed_tailroom = MACSEC_NEEDED_TAILROOM;
>     	}
>
>     	dev->needed_headroom = real_dev->needed_headroom + needed_headroom;
>     	dev->needed_tailroom = real_dev->needed_tailroom + needed_tailroom;
>     }

--
Thanks,

Rahul Rameshbabu