netdev - Re: [RFC PATCH net-next v2 2/2] net: veth: Improving page pool pages recycling

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Mon, 7 Aug 2023 20:20:25 +0800
From: Liang Chen <liangchen.linux@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, hawk@...nel.org, ilias.apalodimas@...aro.org, 
	daniel@...earbox.net, ast@...nel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next v2 2/2] net: veth: Improving page pool pages recycling

On Wed, Aug 2, 2023 at 8:32 PM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2023/8/1 14:19, Liang Chen wrote:
>
> > @@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
> >       case XDP_PASS:
> >               break;
> >       case XDP_TX:
> > -             veth_xdp_get(xdp);
> > -             consume_skb(skb);
> > -             xdp->rxq->mem = rq->xdp_mem;
> > +             if (skb != skb_orig) {
> > +                     xdp->rxq->mem = rq->xdp_mem_pp;
> > +                     kfree_skb_partial(skb, true);
>
> For this case, I suppose that we can safely call kfree_skb_partial()
> as we allocate the skb in the veth_convert_skb_to_xdp_buff(), but
> I am not sure about the !skb->pp_recycle case.
>
> > +             } else if (!skb->pp_recycle) {
> > +                     xdp->rxq->mem = rq->xdp_mem;
> > +                     kfree_skb_partial(skb, true);
>
> For consume_skb(), there is skb_unref() checking and other checking/operation.
> Can we really assume that we can call kfree_skb_partial() with head_stolen
> being true? Is it possible that skb->users is bigger than 1? If it is possible,
> don't we free the 'skb' back to skbuff_cache when other may still be using
> it?
>

Thanks for raising the concern. If there are multiple references to
the skb (skb->users is greater than 1), the skb will be reallocated in
veth_convert_skb_to_xdp_buff(). So it should enter the skb != skb_orig
case.

In fact, entering the !skb->pp_recycle case implies that the skb meets
the following conditions:
1. It is neither shared nor cloned.
2. It is not allocated using kmalloc.
3. It does not have fragment data.
4. The headroom of the skb is greater than XDP_PACKET_HEADROOM.


Thanks,
Liang

> > +             } else {
> > +                     veth_xdp_get(xdp);
> > +                     consume_skb(skb);
> > +                     xdp->rxq->mem = rq->xdp_mem;
> > +             }
> > +
>