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

[PATCH net-next v2 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: 6f13bec53a48c7120dc6dc358cacea13251a471f
-- 
2.43.0

[PATCH net-next v2 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 as this is already handled by XDP during
xdp_rxq_info_unreg (Rx queue unregister), failing to do will cause the
following warning:

warning logs: https://gist.github.com/MeghanaMalladiTI/eb627e5dc8de24e42d7d46572c13e576

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

Changes from v1(v2-v1):
- Included more trace logs for the warning
- Collected RB tag from Simon Horman <horms@kernel.org>

 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

[PATCH net-next v2 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>
Reviewed-by: Simon Horman <horms@kernel.org>
---

Changes from v1(v2-v1):
- Collected RB tag from Simon Horman <horms@kernel.org>

 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

[PATCH net-next v2 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>
Reviewed-by: Simon Horman <horms@kernel.org>
---

Changes from v1(v2-v1):
- Collected RB tag from Simon Horman <horms@kernel.org>

 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 v2 0/3] Bug fixes from XDP and perout series

[Jakub Kicinski]

On Fri, 21 Mar 2025 13:43:10 +0530 Meghana Malladi wrote:
> 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.

Could you perhaps add the customary

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>

tags, then?

Please also link to the reports.

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

[Jakub Kicinski]

On Fri, 21 Mar 2025 13:43:13 +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.

This doesn't really explain the condition under which the bug triggers.
Presumably when user request comes in req is never NULL?

> 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>
> ---
> 
> Changes from v1(v2-v1):
> - Collected RB tag from Simon Horman <horms@kernel.org>
> 
>  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 */

code is trivial here, explain the 'why' not the 'what'
Why is this called with NULL?

> +	if (!req)
> +		return -EINVAL;
-- 
pw-bot: cr

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

[Malladi, Meghana]

On 3/25/2025 11:14 PM, Jakub Kicinski wrote:
> On Fri, 21 Mar 2025 13:43:10 +0530 Meghana Malladi wrote:
>> 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.
> 
> Could you perhaps add the customary
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> tags, then?
> 
> Please also link to the reports.

Sure, I will add them and post to the v3 to net.

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

[Malladi, Meghana]

On 3/25/2025 11:18 PM, Jakub Kicinski wrote:
> On Fri, 21 Mar 2025 13:43:13 +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.
> 
> This doesn't really explain the condition under which the bug triggers.
> Presumably when user request comes in req is never NULL?
> 

You are right, I have looked into what would trigger this bug but seems 
like user request can never be NULL, but the contents inside the req can 
be invalid, but that is already being handled by the kernel. So this bug 
fix makes no sense and I will be dropping this patch for v3. Thanks.

>> 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>
>> ---
>>
>> Changes from v1(v2-v1):
>> - Collected RB tag from Simon Horman <horms@kernel.org>
>>
>>   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 */
> 
> code is trivial here, explain the 'why' not the 'what'
> Why is this called with NULL?
> 
>> +	if (!req)
>> +		return -EINVAL;

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

[Dan Carpenter]

On Fri, Mar 28, 2025 at 11:46:49AM +0530, Malladi, Meghana wrote:
> 
> 
> On 3/25/2025 11:18 PM, Jakub Kicinski wrote:
> > On Fri, 21 Mar 2025 13:43:13 +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.
> > 
> > This doesn't really explain the condition under which the bug triggers.
> > Presumably when user request comes in req is never NULL?
> > 
> 
> You are right, I have looked into what would trigger this bug but seems like
> user request can never be NULL, but the contents inside the req can be
> invalid, but that is already being handled by the kernel. So this bug fix
> makes no sense and I will be dropping this patch for v3. Thanks.
> 

I don't remember bug reports for more than a few hours so I had to dig
this up on lore:

https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/

This is definitely still a real bug on today's linux-next but yes, the
fix is bad.

drivers/net/ethernet/ti/icssg/icss_iep.c
   814  int icss_iep_exit(struct icss_iep *iep)
   815  {
   816          if (iep->ptp_clock) {
   817                  ptp_clock_unregister(iep->ptp_clock);
   818                  iep->ptp_clock = NULL;
   819          }
   820          icss_iep_disable(iep);
   821  
   822          if (iep->pps_enabled)
   823                  icss_iep_pps_enable(iep, false);
   824          else if (iep->perout_enabled)
   825                  icss_iep_perout_enable(iep, NULL, false);
                                                    ^^^^
A better fix probably to delete this function call instead of
turning it into a no-op.

   826  
   827          return 0;
   828  }

regards,
dan carpenter

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

[Malladi, Meghana]

On 3/28/2025 1:32 PM, Dan Carpenter wrote:
> On Fri, Mar 28, 2025 at 11: 46: 49AM +0530, Malladi, Meghana wrote: > > 
>  > On 3/25/2025 11: 18 PM, Jakub Kicinski wrote: > > On Fri, 21 Mar 2025 
> 13: 43: 13 +0530 Meghana Malladi wrote: > > > Whenever there is a perout 
> request
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! 
> uldqfVdF3CcxCkXE1pKJH1UXrlua- 
> GvI0DSoHvw1l5Yyu6xCbgDWX8PlhMSuV5lCv48fT7ChvXbQ105c6PHChn2lWCWZMsMcoHjvVwM$>
> ZjQcmQRYFpfptBannerEnd
> 
> On Fri, Mar 28, 2025 at 11:46:49AM +0530, Malladi, Meghana wrote:
>> 
>> 
>> On 3/25/2025 11:18 PM, Jakub Kicinski wrote:
>> > On Fri, 21 Mar 2025 13:43:13 +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.
>> > 
>> > This doesn't really explain the condition under which the bug triggers.
>> > Presumably when user request comes in req is never NULL?
>> > 
>> 
>> You are right, I have looked into what would trigger this bug but seems like
>> user request can never be NULL, but the contents inside the req can be
>> invalid, but that is already being handled by the kernel. So this bug fix
>> makes no sense and I will be dropping this patch for v3. Thanks.
>> 
> 
> I don't remember bug reports for more than a few hours so I had to dig
> this up on lore:
> 
> https://urldefense.com/v3/__https://lore.kernel.org/ 
> all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/__;!!G3vK! 
> XPLUWNuB49W9FXpnac95FM8thR9_zMOBwt7JgYy8Yaf72LIm4Xt-3Yc8h1sEti5uVdguSWQhcfsnC1_ymQIMTg$ <https://urldefense.com/v3/__https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/__;!!G3vK!XPLUWNuB49W9FXpnac95FM8thR9_zMOBwt7JgYy8Yaf72LIm4Xt-3Yc8h1sEti5uVdguSWQhcfsnC1_ymQIMTg$>
> 
> This is definitely still a real bug on today's linux-next but yes, the
> fix is bad.
> 
> drivers/net/ethernet/ti/icssg/icss_iep.c
>     814  int icss_iep_exit(struct icss_iep *iep)
>     815  {
>     816          if (iep->ptp_clock) {
>     817                  ptp_clock_unregister(iep->ptp_clock);
>     818                  iep->ptp_clock = NULL;
>     819          }
>     820          icss_iep_disable(iep);
>     821
>     822          if (iep->pps_enabled)
>     823                  icss_iep_pps_enable(iep, false);
>     824          else if (iep->perout_enabled)
>     825                  icss_iep_perout_enable(iep, NULL, false);
>                                                      ^^^^
> A better fix probably to delete this function call instead of
> turning it into a no-op.

Yes agreed, current bug fix is bad. Will have a fix similar to this and 
post it soon. Thanks.

> 
>     826
>     827          return 0;
>     828  }
> 
> regards,
> dan carpenter
>