* [PATCH] net: netcp: Fix crash in error path when DMA channel open fails
@ 2025-09-26 15:08 Nishanth Menon
2025-09-26 16:13 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: Nishanth Menon @ 2025-09-26 15:08 UTC (permalink / raw)
To: Uwe Kleine-König, Paolo Abeni, Jakub Kicinski, Eric Dumazet,
David S. Miller, Andrew Lunn
Cc: linux-kernel, netdev, Vadapalli, Siddharth, Nishanth Menon
When knav_dma_open_channel() fails in netcp_setup_navigator_resources(),
the rx_channel field is set to an ERR_PTR value. Later, when
netcp_free_navigator_resources() is called in the error path, it attempts
to close this invalid channel pointer, causing a crash.
Add a check for ERR values to handle the failure scenario.
Fixes: 84640e27f230 ("net: netcp: Add Keystone NetCP core driver")
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Seen on kci log for k2hk: https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz
drivers/net/ethernet/ti/netcp_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 857820657bac..4ff17fd6caae 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1549,7 +1549,7 @@ static void netcp_free_navigator_resources(struct netcp_intf *netcp)
{
int i;
- if (netcp->rx_channel) {
+ if (!IS_ERR(netcp->rx_channel)) {
knav_dma_close_channel(netcp->rx_channel);
netcp->rx_channel = NULL;
}
--
2.47.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: netcp: Fix crash in error path when DMA channel open fails 2025-09-26 15:08 [PATCH] net: netcp: Fix crash in error path when DMA channel open fails Nishanth Menon @ 2025-09-26 16:13 ` Simon Horman 2025-09-26 16:27 ` Siddharth Vadapalli 0 siblings, 1 reply; 6+ messages in thread From: Simon Horman @ 2025-09-26 16:13 UTC (permalink / raw) To: Nishanth Menon Cc: Uwe Kleine-König, Paolo Abeni, Jakub Kicinski, Eric Dumazet, David S. Miller, Andrew Lunn, linux-kernel, netdev, Vadapalli, Siddharth On Fri, Sep 26, 2025 at 10:08:53AM -0500, Nishanth Menon wrote: > When knav_dma_open_channel() fails in netcp_setup_navigator_resources(), > the rx_channel field is set to an ERR_PTR value. Later, when > netcp_free_navigator_resources() is called in the error path, it attempts > to close this invalid channel pointer, causing a crash. > > Add a check for ERR values to handle the failure scenario. > > Fixes: 84640e27f230 ("net: netcp: Add Keystone NetCP core driver") > Signed-off-by: Nishanth Menon <nm@ti.com> > --- > > Seen on kci log for k2hk: https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz > > drivers/net/ethernet/ti/netcp_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c > index 857820657bac..4ff17fd6caae 100644 > --- a/drivers/net/ethernet/ti/netcp_core.c > +++ b/drivers/net/ethernet/ti/netcp_core.c > @@ -1549,7 +1549,7 @@ static void netcp_free_navigator_resources(struct netcp_intf *netcp) > { > int i; > > - if (netcp->rx_channel) { > + if (!IS_ERR(netcp->rx_channel)) { > knav_dma_close_channel(netcp->rx_channel); > netcp->rx_channel = NULL; > } Hi Nishanth, Thanks for your patch. I expect that netcp_txpipe_close() has a similar problem too. But I also think that using IS_ERR is not correct, because it seems to me that there are also cases where rx_channel can be NULL. I see that on error knav_dma_open_channel() always returns ERR_PTR(-EINVAL) (open coded as (void *)-EINVAL) on error. So I think a better approach would be to change knav_dma_open_channel() to return NULL, and update callers accordingly. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: netcp: Fix crash in error path when DMA channel open fails 2025-09-26 16:13 ` Simon Horman @ 2025-09-26 16:27 ` Siddharth Vadapalli 2025-09-26 16:42 ` Simon Horman 0 siblings, 1 reply; 6+ messages in thread From: Siddharth Vadapalli @ 2025-09-26 16:27 UTC (permalink / raw) To: Simon Horman, Nishanth Menon Cc: Uwe Kleine-König, Paolo Abeni, Jakub Kicinski, Eric Dumazet, David S. Miller, Andrew Lunn, linux-kernel, netdev, s-vadapalli On 26/09/25 9:43 PM, Simon Horman wrote: > On Fri, Sep 26, 2025 at 10:08:53AM -0500, Nishanth Menon wrote: >> When knav_dma_open_channel() fails in netcp_setup_navigator_resources(), >> the rx_channel field is set to an ERR_PTR value. Later, when >> netcp_free_navigator_resources() is called in the error path, it attempts >> to close this invalid channel pointer, causing a crash. >> >> Add a check for ERR values to handle the failure scenario. >> >> Fixes: 84640e27f230 ("net: netcp: Add Keystone NetCP core driver") >> Signed-off-by: Nishanth Menon <nm@ti.com> >> --- >> >> Seen on kci log for k2hk: https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz >> >> drivers/net/ethernet/ti/netcp_core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c >> index 857820657bac..4ff17fd6caae 100644 >> --- a/drivers/net/ethernet/ti/netcp_core.c >> +++ b/drivers/net/ethernet/ti/netcp_core.c >> @@ -1549,7 +1549,7 @@ static void netcp_free_navigator_resources(struct netcp_intf *netcp) >> { >> int i; >> >> - if (netcp->rx_channel) { >> + if (!IS_ERR(netcp->rx_channel)) { >> knav_dma_close_channel(netcp->rx_channel); >> netcp->rx_channel = NULL; >> } > > Hi Nishanth, > > Thanks for your patch. > > I expect that netcp_txpipe_close() has a similar problem too. > > But I also think that using IS_ERR is not correct, because it seems to me > that there are also cases where rx_channel can be NULL. Could you please clarify where rx_channel is NULL? rx_channel is set by invoking knav_dma_open_channel(). Also, please refer to: https://github.com/torvalds/linux/commit/5b6cb43b4d62 which specifically points out that knav_dma_open_channel() will not return NULL so the check for NULL isn't required. > > I see that on error knav_dma_open_channel() always returns ERR_PTR(-EINVAL) > (open coded as (void *)-EINVAL) on error. So I think a better approach > would be to change knav_dma_open_channel() to return NULL, and update callers > accordingly. The commit referred to above made changes to the driver specifically due to the observation that knav_dma_open_channel() never returns NULL. Modifying knav_dma_open_channel() to return NULL will effectively result in having to undo the changes made by the commit. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: netcp: Fix crash in error path when DMA channel open fails 2025-09-26 16:27 ` Siddharth Vadapalli @ 2025-09-26 16:42 ` Simon Horman 2025-09-26 16:58 ` Siddharth Vadapalli 0 siblings, 1 reply; 6+ messages in thread From: Simon Horman @ 2025-09-26 16:42 UTC (permalink / raw) To: Siddharth Vadapalli Cc: Nishanth Menon, Uwe Kleine-König, Paolo Abeni, Jakub Kicinski, Eric Dumazet, David S. Miller, Andrew Lunn, linux-kernel, netdev On Fri, Sep 26, 2025 at 09:57:02PM +0530, Siddharth Vadapalli wrote: > On 26/09/25 9:43 PM, Simon Horman wrote: > > On Fri, Sep 26, 2025 at 10:08:53AM -0500, Nishanth Menon wrote: > > > When knav_dma_open_channel() fails in netcp_setup_navigator_resources(), > > > the rx_channel field is set to an ERR_PTR value. Later, when > > > netcp_free_navigator_resources() is called in the error path, it attempts > > > to close this invalid channel pointer, causing a crash. > > > > > > Add a check for ERR values to handle the failure scenario. > > > > > > Fixes: 84640e27f230 ("net: netcp: Add Keystone NetCP core driver") > > > Signed-off-by: Nishanth Menon <nm@ti.com> > > > --- > > > > > > Seen on kci log for k2hk: https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz > > > > > > drivers/net/ethernet/ti/netcp_core.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c > > > index 857820657bac..4ff17fd6caae 100644 > > > --- a/drivers/net/ethernet/ti/netcp_core.c > > > +++ b/drivers/net/ethernet/ti/netcp_core.c > > > @@ -1549,7 +1549,7 @@ static void netcp_free_navigator_resources(struct netcp_intf *netcp) > > > { > > > int i; > > > - if (netcp->rx_channel) { > > > + if (!IS_ERR(netcp->rx_channel)) { > > > knav_dma_close_channel(netcp->rx_channel); > > > netcp->rx_channel = NULL; > > > } > > > > Hi Nishanth, > > > > Thanks for your patch. > > > > I expect that netcp_txpipe_close() has a similar problem too. > > > > But I also think that using IS_ERR is not correct, because it seems to me > > that there are also cases where rx_channel can be NULL. > > Could you please clarify where rx_channel is NULL? rx_channel is set by > invoking knav_dma_open_channel(). Hi Siddharth, I am assuming that when netcp_setup_navigator_resources() is called, at least for the first time, that netcp->rx_channel is NULL. So any of the occurrence of 'goto fail' in that function before the call to knav_dma_open_channel(). > Also, please refer to: > https://github.com/torvalds/linux/commit/5b6cb43b4d62 > which specifically points out that knav_dma_open_channel() will not return > NULL so the check for NULL isn't required. > > > > I see that on error knav_dma_open_channel() always returns ERR_PTR(-EINVAL) > > (open coded as (void *)-EINVAL) on error. So I think a better approach > > would be to change knav_dma_open_channel() to return NULL, and update callers > > accordingly. > > The commit referred to above made changes to the driver specifically due to > the observation that knav_dma_open_channel() never returns NULL. Modifying > knav_dma_open_channel() to return NULL will effectively result in having to > undo the changes made by the commit. I wasn't aware of that patch. But my observation is that the return value of knav_dma_open_channel() is still not handled correctly. E.g. the bug your patch is fixing. And I'm proposing an alternate approach which I feel will be less error-prone. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: netcp: Fix crash in error path when DMA channel open fails 2025-09-26 16:42 ` Simon Horman @ 2025-09-26 16:58 ` Siddharth Vadapalli 2025-09-29 9:59 ` Simon Horman 0 siblings, 1 reply; 6+ messages in thread From: Siddharth Vadapalli @ 2025-09-26 16:58 UTC (permalink / raw) To: Simon Horman Cc: Nishanth Menon, Uwe Kleine-König, Paolo Abeni, Jakub Kicinski, Eric Dumazet, David S. Miller, Andrew Lunn, linux-kernel, netdev, s-vadapalli On 26/09/25 10:12 PM, Simon Horman wrote: > On Fri, Sep 26, 2025 at 09:57:02PM +0530, Siddharth Vadapalli wrote: >> On 26/09/25 9:43 PM, Simon Horman wrote: >>> On Fri, Sep 26, 2025 at 10:08:53AM -0500, Nishanth Menon wrote: >>>> When knav_dma_open_channel() fails in netcp_setup_navigator_resources(), >>>> the rx_channel field is set to an ERR_PTR value. Later, when >>>> netcp_free_navigator_resources() is called in the error path, it attempts >>>> to close this invalid channel pointer, causing a crash. >>>> >>>> Add a check for ERR values to handle the failure scenario. >>>> >>>> Fixes: 84640e27f230 ("net: netcp: Add Keystone NetCP core driver") >>>> Signed-off-by: Nishanth Menon <nm@ti.com> >>>> --- >>>> >>>> Seen on kci log for k2hk: https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz >>>> >>>> drivers/net/ethernet/ti/netcp_core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c >>>> index 857820657bac..4ff17fd6caae 100644 >>>> --- a/drivers/net/ethernet/ti/netcp_core.c >>>> +++ b/drivers/net/ethernet/ti/netcp_core.c >>>> @@ -1549,7 +1549,7 @@ static void netcp_free_navigator_resources(struct netcp_intf *netcp) >>>> { >>>> int i; >>>> - if (netcp->rx_channel) { >>>> + if (!IS_ERR(netcp->rx_channel)) { >>>> knav_dma_close_channel(netcp->rx_channel); >>>> netcp->rx_channel = NULL; >>>> } >>> >>> Hi Nishanth, >>> >>> Thanks for your patch. >>> >>> I expect that netcp_txpipe_close() has a similar problem too. >>> >>> But I also think that using IS_ERR is not correct, because it seems to me >>> that there are also cases where rx_channel can be NULL. >> >> Could you please clarify where rx_channel is NULL? rx_channel is set by >> invoking knav_dma_open_channel(). > > Hi Siddharth, > > I am assuming that when netcp_setup_navigator_resources() is called, at > least for the first time, that netcp->rx_channel is NULL. So any of the > occurrence of 'goto fail' in that function before the call to > knav_dma_open_channel(). I missed this. Thank you for pointing this out. > >> Also, please refer to: >> https://github.com/torvalds/linux/commit/5b6cb43b4d62 >> which specifically points out that knav_dma_open_channel() will not return >> NULL so the check for NULL isn't required. >>> >>> I see that on error knav_dma_open_channel() always returns ERR_PTR(-EINVAL) >>> (open coded as (void *)-EINVAL) on error. So I think a better approach >>> would be to change knav_dma_open_channel() to return NULL, and update callers >>> accordingly. >> >> The commit referred to above made changes to the driver specifically due to >> the observation that knav_dma_open_channel() never returns NULL. Modifying >> knav_dma_open_channel() to return NULL will effectively result in having to >> undo the changes made by the commit. > > I wasn't aware of that patch. But my observation is that the return value > of knav_dma_open_channel() is still not handled correctly. E.g. the bug > your patch is fixing. And I'm proposing an alternate approach which I feel > will be less error-prone. Ok. If I understand correctly, you are proposing that the 'error codes' returned by knav_dma_open_channel() should be turned into a dev_err() print for the user and knav_dma_open_channel() should always return NULL in case of failure and a pointer to the channel in case of success. Is that right? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: netcp: Fix crash in error path when DMA channel open fails 2025-09-26 16:58 ` Siddharth Vadapalli @ 2025-09-29 9:59 ` Simon Horman 0 siblings, 0 replies; 6+ messages in thread From: Simon Horman @ 2025-09-29 9:59 UTC (permalink / raw) To: Siddharth Vadapalli Cc: Nishanth Menon, Uwe Kleine-König, Paolo Abeni, Jakub Kicinski, Eric Dumazet, David S. Miller, Andrew Lunn, linux-kernel, netdev On Fri, Sep 26, 2025 at 10:28:47PM +0530, Siddharth Vadapalli wrote: > On 26/09/25 10:12 PM, Simon Horman wrote: > > On Fri, Sep 26, 2025 at 09:57:02PM +0530, Siddharth Vadapalli wrote: > > > On 26/09/25 9:43 PM, Simon Horman wrote: > > > > On Fri, Sep 26, 2025 at 10:08:53AM -0500, Nishanth Menon wrote: > > > > > When knav_dma_open_channel() fails in netcp_setup_navigator_resources(), > > > > > the rx_channel field is set to an ERR_PTR value. Later, when > > > > > netcp_free_navigator_resources() is called in the error path, it attempts > > > > > to close this invalid channel pointer, causing a crash. > > > > > > > > > > Add a check for ERR values to handle the failure scenario. > > > > > > > > > > Fixes: 84640e27f230 ("net: netcp: Add Keystone NetCP core driver") > > > > > Signed-off-by: Nishanth Menon <nm@ti.com> > > > > > --- > > > > > > > > > > Seen on kci log for k2hk: https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz > > > > > > > > > > drivers/net/ethernet/ti/netcp_core.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c > > > > > index 857820657bac..4ff17fd6caae 100644 > > > > > --- a/drivers/net/ethernet/ti/netcp_core.c > > > > > +++ b/drivers/net/ethernet/ti/netcp_core.c > > > > > @@ -1549,7 +1549,7 @@ static void netcp_free_navigator_resources(struct netcp_intf *netcp) > > > > > { > > > > > int i; > > > > > - if (netcp->rx_channel) { > > > > > + if (!IS_ERR(netcp->rx_channel)) { > > > > > knav_dma_close_channel(netcp->rx_channel); > > > > > netcp->rx_channel = NULL; > > > > > } > > > > > > > > Hi Nishanth, > > > > > > > > Thanks for your patch. > > > > > > > > I expect that netcp_txpipe_close() has a similar problem too. > > > > > > > > But I also think that using IS_ERR is not correct, because it seems to me > > > > that there are also cases where rx_channel can be NULL. > > > > > > Could you please clarify where rx_channel is NULL? rx_channel is set by > > > invoking knav_dma_open_channel(). > > > > Hi Siddharth, > > > > I am assuming that when netcp_setup_navigator_resources() is called, at > > least for the first time, that netcp->rx_channel is NULL. So any of the > > occurrence of 'goto fail' in that function before the call to > > knav_dma_open_channel(). > > I missed this. Thank you for pointing this out. No problem. These error paths are tricking things. > > > Also, please refer to: > > > https://github.com/torvalds/linux/commit/5b6cb43b4d62 > > > which specifically points out that knav_dma_open_channel() will not return > > > NULL so the check for NULL isn't required. > > > > > > > > I see that on error knav_dma_open_channel() always returns ERR_PTR(-EINVAL) > > > > (open coded as (void *)-EINVAL) on error. So I think a better approach > > > > would be to change knav_dma_open_channel() to return NULL, and update callers > > > > accordingly. > > > > > > The commit referred to above made changes to the driver specifically due to > > > the observation that knav_dma_open_channel() never returns NULL. Modifying > > > knav_dma_open_channel() to return NULL will effectively result in having to > > > undo the changes made by the commit. > > > > I wasn't aware of that patch. But my observation is that the return value > > of knav_dma_open_channel() is still not handled correctly. E.g. the bug > > your patch is fixing. And I'm proposing an alternate approach which I feel > > will be less error-prone. > > Ok. If I understand correctly, you are proposing that the 'error codes' > returned by knav_dma_open_channel() should be turned into a dev_err() print > for the user and knav_dma_open_channel() should always return NULL in case > of failure and a pointer to the channel in case of success. Is that right? I'm ambivalent regarding the dev_err() part. Because the error is always the same. And I'm not really sure that logging it adds anything. But if you do go that way, please consider using %pe. consider using Regarding knav_dma_open_channel(0 always returning NULL, yes, that is my suggestion. Of course the callers and anything else that uses that return value need to be audited and updated as appropriate. Thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-29 9:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-26 15:08 [PATCH] net: netcp: Fix crash in error path when DMA channel open fails Nishanth Menon 2025-09-26 16:13 ` Simon Horman 2025-09-26 16:27 ` Siddharth Vadapalli 2025-09-26 16:42 ` Simon Horman 2025-09-26 16:58 ` Siddharth Vadapalli 2025-09-29 9:59 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).