[PATCH] net: page_pool: check nla_nest_start() return value in page_pool_nl_stats_fill()

[PATCH] net: page_pool: check nla_nest_start() return value in page_pool_nl_stats_fill()

[Zhao Dongdong]

From: Zhao Dongdong <zhaodongdong@kylinos.cn>

nla_nest_start() can return NULL if the skb runs out of space.
page_pool_nl_stats_fill() does not check the return value before
calling nla_nest_end(), which can lead to a NULL pointer dereference.
Add a NULL check after nla_nest_start() and abort the message if it fails.

Fixes: d49010adae73 ("net: page_pool: expose page pool stats via netlink")
Signed-off-by: Zhao Dongdong <zhaodongdong@kylinos.cn>
---
 net/core/page_pool_user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/page_pool_user.c b/net/core/page_pool_user.c
index ee5060d8eec0..2e41691251e1 100644
--- a/net/core/page_pool_user.c
+++ b/net/core/page_pool_user.c
@@ -127,6 +127,8 @@ page_pool_nl_stats_fill(struct sk_buff *rsp, const struct page_pool *pool,
 		return -EMSGSIZE;
 
 	nest = nla_nest_start(rsp, NETDEV_A_PAGE_POOL_STATS_INFO);
+	if (!nest)
+		goto err_cancel_msg;
 
 	if (nla_put_uint(rsp, NETDEV_A_PAGE_POOL_ID, pool->user.id) ||
 	    (pool->slow.netdev->ifindex != LOOPBACK_IFINDEX &&
-- 
2.25.1

Re: [PATCH] net: page_pool: check nla_nest_start() return value in page_pool_nl_stats_fill()

[Alexander Lobakin]

From: Zhao Dongdong <winter91@foxmail.com>
Date: Tue, 26 May 2026 14:51:56 +0800

> From: Zhao Dongdong <zhaodongdong@kylinos.cn>
> 
> nla_nest_start() can return NULL if the skb runs out of space.
> page_pool_nl_stats_fill() does not check the return value before
> calling nla_nest_end(), which can lead to a NULL pointer dereference.
> Add a NULL check after nla_nest_start() and abort the message if it fails.
> 
> Fixes: d49010adae73 ("net: page_pool: expose page pool stats via netlink")

A "Cc: stable@vger.kernel.org" candidate?

> Signed-off-by: Zhao Dongdong <zhaodongdong@kylinos.cn>

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Thanks,
Olek

Re: [PATCH] net: page_pool: check nla_nest_start() return value in page_pool_nl_stats_fill()

[Jakub Kicinski]

On Tue, 26 May 2026 18:04:44 +0200 Alexander Lobakin wrote:
> From: Zhao Dongdong <winter91@foxmail.com>
> Date: Tue, 26 May 2026 14:51:56 +0800
> 
> > From: Zhao Dongdong <zhaodongdong@kylinos.cn>
> > 
> > nla_nest_start() can return NULL if the skb runs out of space.
> > page_pool_nl_stats_fill() does not check the return value before
> > calling nla_nest_end(), which can lead to a NULL pointer dereference.
> > Add a NULL check after nla_nest_start() and abort the message if it fails.
> > 
> > Fixes: d49010adae73 ("net: page_pool: expose page pool stats via netlink")  
> 
> A "Cc: stable@vger.kernel.org" candidate?

Not really, the thinking is that if nla_nest_start() failed then the
very next nla_put* will also fail (nla_nest_start() is a zero-length
attr, any attr we put after must be >= in length).

IOW the commit message is lying, there's no way for us to reach
nla_nest_end(). We will jump to nla_nest_cancel() which handles
nest=NULL just fine.

Zhao Dongdong please confirm this and respin the patch without a Fixes 
tag and with an updated commit message. I think we got such a patch
in the past, so we should merge it to avoid confusing bots. 
But please make it very clear that this code can't crash..
-- 
pw-bot: cr

Re: [PATCH] net: page_pool: check nla_nest_start() return value in

[Zhao Dongdong]

On Tue, 26 May 2026 16:27:36 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 26 May 2026 18:04:44 +0200 Alexander Lobakin wrote:
> > From: Zhao Dongdong <winter91@foxmail.com>
> > Date: Tue, 26 May 2026 14:51:56 +0800
> > 
> > > From: Zhao Dongdong <zhaodongdong@kylinos.cn>
> > > 
> > > nla_nest_start() can return NULL if the skb runs out of space.
> > > page_pool_nl_stats_fill() does not check the return value before
> > > calling nla_nest_end(), which can lead to a NULL pointer dereference.
> > > Add a NULL check after nla_nest_start() and abort the message if it fails.
> > > 
> > > Fixes: d49010adae73 ("net: page_pool: expose page pool stats via netlink")  
> > 
> > A "Cc: stable@vger.kernel.org" candidate?
>
> Not really, the thinking is that if nla_nest_start() failed then the
> very next nla_put* will also fail (nla_nest_start() is a zero-length
> attr, any attr we put after must be >= in length).
>
> IOW the commit message is lying, there's no way for us to reach
> nla_nest_end(). We will jump to nla_nest_cancel() which handles
> nest=NULL just fine.
>
> Zhao Dongdong please confirm this and respin the patch without a Fixes 
> tag and with an updated commit message. I think we got such a patch
> in the past, so we should merge it to avoid confusing bots. 
> But please make it very clear that this code can't crash..
> -- 
> pw-bot: cr

Thanks for your review.
I carefully reviewed the code. Your thinking should be right --I just happened to notice that most call sites of nla_nest_start() include a non-NULL check. Apologies for the noise.


-- 
Regards,
    Zhao Dongdong