netdev - Re: [RFC PATCH v2 11/21] core/skbuff: add page recycling logic for netgpu pages

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:   Tue, 28 Jul 2020 18:28:25 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [RFC PATCH v2 11/21] core/skbuff: add page recycling logic for
 netgpu pages

On Mon, Jul 27, 2020 at 03:44:34PM -0700, Jonathan Lemon wrote:
> From: Jonathan Lemon <bsd@...com>
> 
> netgpu pages will always have a refcount of at least one (held by
> the netgpu module).  If the skb is marked as containing netgpu ZC
> pages, recycle them back to netgpu.

What???

> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
> ---
>  net/core/skbuff.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 1422b99b7090..50dbb7ce1965 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -591,6 +591,27 @@ static void skb_free_head(struct sk_buff *skb)
>  		kfree(head);
>  }
>  
> +#if IS_ENABLED(CONFIG_NETGPU)
> +static void skb_netgpu_unref(struct skb_shared_info *shinfo)
> +{
> +	struct netgpu_ifq *ifq = shinfo->destructor_arg;
> +	struct page *page;
> +	int i;
> +
> +	/* pages attached for skbs for TX shouldn't come here, since
> +	 * the skb is not marked as "zc_netgpu". (only RX skbs have this).
> +	 * dummy page does come here, but always has elevated refc.
> +	 *
> +	 * Undelivered zc skb's will arrive at this point.
> +	 */
> +	for (i = 0; i < shinfo->nr_frags; i++) {
> +		page = skb_frag_page(&shinfo->frags[i]);
> +		if (page && page_ref_dec_return(page) <= 2)
> +			netgpu_put_page(ifq, page, false);
> +	}
> +}
> +#endif

Becides the basic "no #if in C files" issue here, why is this correct?

> +
>  static void skb_release_data(struct sk_buff *skb)
>  {
>  	struct skb_shared_info *shinfo = skb_shinfo(skb);
> @@ -601,8 +622,15 @@ static void skb_release_data(struct sk_buff *skb)
>  			      &shinfo->dataref))
>  		return;
>  
> -	for (i = 0; i < shinfo->nr_frags; i++)
> -		__skb_frag_unref(&shinfo->frags[i]);
> +#if IS_ENABLED(CONFIG_NETGPU)
> +	if (skb->zc_netgpu && shinfo->nr_frags) {
> +		skb_netgpu_unref(shinfo);
> +	} else
> +#endif
> +	{
> +		for (i = 0; i < shinfo->nr_frags; i++)
> +			__skb_frag_unref(&shinfo->frags[i]);
> +	}

Again, no #if in C code.  But even then, this feels really really wrong.

greg k-h