[PATCH net-next 0/3] Bug fixes from XDP and perout series

[PATCH net-next 0/3] Bug fixes from XDP and perout series

[Meghana Malladi]

This patch series consists of bug fixes from the features which recently
got merged into net-next, and haven't been merged to net tree yet.

Patch 2/3 and 3/3 are bug fixes reported by Dan Carpenter
<dan.carpenter@linaro.org> using Smatch static checker.

Meghana Malladi (3):
  net: ti: prueth: Fix kernel warning while bringing down network
    interface
  net: ti: prueth: Fix possible NULL pointer dereference inside
    emac_xmit_xdp_frame()
  net: ti: icss-iep: Fix possible NULL pointer dereference for perout
    request

 drivers/net/ethernet/ti/icssg/icss_iep.c     | 4 ++++
 drivers/net/ethernet/ti/icssg/icssg_common.c | 9 ++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)


base-commit: bfc6c67ec2d64d0ca4e5cc3e1ac84298a10b8d62
-- 
2.43.0

[PATCH net-next 1/3] net: ti: prueth: Fix kernel warning while bringing down network interface

[Meghana Malladi]

During network interface initialization, the NIC driver needs to register
its Rx queue with the XDP, to ensure the incoming XDP buffer carries a
pointer reference to this info and is stored inside xdp_rxq_info.

While this struct isn't tied to XDP prog, if there are any changes in
Rx queue, the NIC driver needs to stop the Rx queue by unregistering
with XDP before purging and reallocating memory. Drop page_pool destroy
during Rx channel reset and this is already handled by XDP during
xdp_rxq_info_unreg (Rx queue unregister), failing to do will cause the
following warning:

[  271.494611] ------------[ cut here ]------------
[  271.494629] WARNING: CPU: 0 PID: 2453 at /net/core/page_pool.c:1108 0xffff8000808d5f60

Fixes: 46eeb90f03e0 ("net: ti: icssg-prueth: Use page_pool API for RX buffer allocation")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index df5da7a98abf..afa01c22dee8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -1216,9 +1216,6 @@ void prueth_reset_rx_chan(struct prueth_rx_chn *chn,
 					  prueth_rx_cleanup, !!i);
 	if (disable)
 		k3_udma_glue_disable_rx_chn(chn->rx_chn);
-
-	page_pool_destroy(chn->pg_pool);
-	chn->pg_pool = NULL;
 }
 EXPORT_SYMBOL_GPL(prueth_reset_rx_chan);
 
-- 
2.43.0

Re: [PATCH net-next 1/3] net: ti: prueth: Fix kernel warning while bringing down network interface

[Simon Horman]

On Mon, Mar 17, 2025 at 03:45:48PM +0530, Meghana Malladi wrote:
> During network interface initialization, the NIC driver needs to register
> its Rx queue with the XDP, to ensure the incoming XDP buffer carries a
> pointer reference to this info and is stored inside xdp_rxq_info.
> 
> While this struct isn't tied to XDP prog, if there are any changes in
> Rx queue, the NIC driver needs to stop the Rx queue by unregistering
> with XDP before purging and reallocating memory. Drop page_pool destroy
> during Rx channel reset and this is already handled by XDP during
> xdp_rxq_info_unreg (Rx queue unregister), failing to do will cause the
> following warning:
> 
> [  271.494611] ------------[ cut here ]------------
> [  271.494629] WARNING: CPU: 0 PID: 2453 at /net/core/page_pool.c:1108 0xffff8000808d5f60

I think it would be nice to include a bit more of the stack trace here.

> 
> Fixes: 46eeb90f03e0 ("net: ti: icssg-prueth: Use page_pool API for RX buffer allocation")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

It is a shame that we now have more asymmetry regarding
the allocation of the pool and unwind on error prueth_prepare_rx_chan().

But if I see things correctly the freeing of the pool via
xdp_rxq_info_unreg() is unconditional. And with that in mind
I agree the approach taken by this patch makes sense.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH net-next 1/3] net: ti: prueth: Fix kernel warning while bringing down network interface

[Malladi, Meghana]

Hi Simon,
Thanks for reviewing the patch series.

On 3/20/2025 6:23 PM, Simon Horman wrote:
> On Mon, Mar 17, 2025 at 03:45:48PM +0530, Meghana Malladi wrote:
>> During network interface initialization, the NIC driver needs to register
>> its Rx queue with the XDP, to ensure the incoming XDP buffer carries a
>> pointer reference to this info and is stored inside xdp_rxq_info.
>>
>> While this struct isn't tied to XDP prog, if there are any changes in
>> Rx queue, the NIC driver needs to stop the Rx queue by unregistering
>> with XDP before purging and reallocating memory. Drop page_pool destroy
>> during Rx channel reset and this is already handled by XDP during
>> xdp_rxq_info_unreg (Rx queue unregister), failing to do will cause the
>> following warning:
>>
>> [  271.494611] ------------[ cut here ]------------
>> [  271.494629] WARNING: CPU: 0 PID: 2453 at /net/core/page_pool.c:1108 0xffff8000808d5f60
> 
> I think it would be nice to include a bit more of the stack trace here.
> 

Sure, I will attach a link to the warning logs in v2.

>>
>> Fixes: 46eeb90f03e0 ("net: ti: icssg-prueth: Use page_pool API for RX buffer allocation")
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> 
> It is a shame that we now have more asymmetry regarding
> the allocation of the pool and unwind on error prueth_prepare_rx_chan().
> 
> But if I see things correctly the freeing of the pool via
> xdp_rxq_info_unreg() is unconditional. And with that in mind
> I agree the approach taken by this patch makes sense.
> 

Agreed on the asymmetry part, but I am not quite convinced on why 
xdp_rxq_info_unreg() is freeing the pool unconditionally, when the 
driver is the one which is allocating the pool. If xdp_rxq_info_unreg() 
is freeing it, then shouldn't xdp_rxq_info_reg() be allocating it ?

> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> ...

[PATCH net-next 2/3] net: ti: prueth: Fix possible NULL pointer dereference inside emac_xmit_xdp_frame()

[Meghana Malladi]

There is an error check inside emac_xmit_xdp_frame() function which
is called when the driver wants to transmit XDP frame, to check if
the allocated tx descriptor is NULL, if true to exit and return
ICSSG_XDP_CONSUMED implying failure in transmission.

In this case trying to free a descriptor which is NULL will result
in kernel crash due to NULL pointer dereference. Fix this error handling
and increase netdev tx_dropped stats in the caller of this function
if the function returns ICSSG_XDP_CONSUMED.

Fixes: 62aa3246f462 ("net: ti: icssg-prueth: Add XDP support")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index afa01c22dee8..f0224576b95f 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -584,7 +584,7 @@ u32 emac_xmit_xdp_frame(struct prueth_emac *emac,
 	first_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool);
 	if (!first_desc) {
 		netdev_dbg(ndev, "xdp tx: failed to allocate descriptor\n");
-		goto drop_free_descs;	/* drop */
+		return ICSSG_XDP_CONSUMED;	/* drop */
 	}
 
 	if (page) { /* already DMA mapped by page_pool */
@@ -672,8 +672,10 @@ static u32 emac_run_xdp(struct prueth_emac *emac, struct xdp_buff *xdp,
 
 		q_idx = smp_processor_id() % emac->tx_ch_num;
 		result = emac_xmit_xdp_frame(emac, xdpf, page, q_idx);
-		if (result == ICSSG_XDP_CONSUMED)
+		if (result == ICSSG_XDP_CONSUMED) {
+			ndev->stats.tx_dropped++;
 			goto drop;
+		}
 
 		dev_sw_netstats_rx_add(ndev, xdpf->len);
 		return result;
-- 
2.43.0

Re: [PATCH net-next 2/3] net: ti: prueth: Fix possible NULL pointer dereference inside emac_xmit_xdp_frame()

[Simon Horman]

On Mon, Mar 17, 2025 at 03:45:49PM +0530, Meghana Malladi wrote:
> There is an error check inside emac_xmit_xdp_frame() function which
> is called when the driver wants to transmit XDP frame, to check if
> the allocated tx descriptor is NULL, if true to exit and return
> ICSSG_XDP_CONSUMED implying failure in transmission.
> 
> In this case trying to free a descriptor which is NULL will result
> in kernel crash due to NULL pointer dereference. Fix this error handling
> and increase netdev tx_dropped stats in the caller of this function
> if the function returns ICSSG_XDP_CONSUMED.
> 
> Fixes: 62aa3246f462 ("net: ti: icssg-prueth: Add XDP support")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request

[Meghana Malladi]

Whenever there is a perout request from the user application,
kernel receives req structure containing the configuration info
for that req. Add NULL pointer handling for perout request if
that req struct points to NULL.

Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index b4a34c57b7b4..aeebdc4c121e 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
 {
 	int ret = 0;
 
+	/* Return error if the req is NULL */
+	if (!req)
+		return -EINVAL;
+
 	/* Reject requests with unsupported flags */
 	if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE |
 			  PTP_PEROUT_PHASE))
-- 
2.43.0

Re: [PATCH net-next 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request

[Simon Horman]

On Mon, Mar 17, 2025 at 03:45:50PM +0530, Meghana Malladi wrote:
> Whenever there is a perout request from the user application,
> kernel receives req structure containing the configuration info
> for that req. Add NULL pointer handling for perout request if
> that req struct points to NULL.
> 
> Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

Reviewed-by: Simon Horman <horms@kernel.org>