netdev - Re: [PATCH net-next v8 4/4] skbuff: Optimization of SKB coalescing for page pool

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]

Message-ID: <20231211121409.5cfaebd5@kernel.org>
Date: Mon, 11 Dec 2023 12:14:09 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc: Liang Chen <liangchen.linux@...il.com>, davem@...emloft.net,
 edumazet@...gle.com, pabeni@...hat.com, hawk@...nel.org,
 linyunsheng@...wei.com, netdev@...r.kernel.org, linux-mm@...ck.org,
 jasowang@...hat.com, almasrymina@...gle.com
Subject: Re: [PATCH net-next v8 4/4] skbuff: Optimization of SKB coalescing
 for page pool

On Mon, 11 Dec 2023 09:46:55 +0200 Ilias Apalodimas wrote:
> As I said in the past the patch look correct. I don't like the fact
> that more pp internals creep into the default network stack, but
> perhaps this is fine with the bigger adoption?
> Jakub any thoughts/objections?

Now that you asked... the helper does seem to be in sort of 
a in-between state of being skb specific.

What worries me is that this:

+/**
+ * skb_pp_frag_ref() - Increase fragment reference count of a page
+ * @page:	page of the fragment on which to increase a reference
+ *
+ * Increase fragment reference count (pp_ref_count) on a page, but if it is
+ * not a page pool page, fallback to increase a reference(_refcount) on a
+ * normal page.
+ */
+static void skb_pp_frag_ref(struct page *page)
+{
+	struct page *head_page = compound_head(page);
+
+	if (likely(is_pp_page(head_page)))
+		page_pool_ref_page(head_page);
+	else
+		page_ref_inc(head_page);
+}

doesn't even document that the caller must make sure that the skb
which owns this page is marked for pp recycling. The caller added
by this patch does that, but we should indicate somewhere that doing
skb_pp_frag_ref() for frag in a non-pp-recycling skb is not correct.

We can either lean in the direction of making it less skb specific,
put the code in page_pool.c / helpers.h and make it clear that the
caller has to be careful.
Or we make it more skb specific, take a skb pointer as arg, and also
look at its recycling marking..
or just improve the kdoc.