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 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.

    		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