* [PATCH AUTOSEL 6.17-5.4] net: ethernet: ti: netcp: Standardize knav_dma_open_channel to return NULL on error
[not found] <20251110195718.859919-1-sashal@kernel.org>
@ 2025-11-10 19:57 ` Sasha Levin
0 siblings, 0 replies; only message in thread
From: Sasha Levin @ 2025-11-10 19:57 UTC (permalink / raw)
To: patches, stable
Cc: Nishanth Menon, Simon Horman, Jacob Keller, Jakub Kicinski,
Sasha Levin, alexandre.f.demers, alexander.deucher, linux-kernel,
linux-arm-kernel
From: Nishanth Menon <nm@ti.com>
[ Upstream commit 90a88306eb874fe4bbdd860e6c9787f5bbc588b5 ]
Make knav_dma_open_channel consistently return NULL on error instead
of ERR_PTR. Currently the header include/linux/soc/ti/knav_dma.h
returns NULL when the driver is disabled, but the driver
implementation does not even return NULL or ERR_PTR on failure,
causing inconsistency in the users. This results in a crash in
netcp_free_navigator_resources as followed (trimmed):
Unhandled fault: alignment exception (0x221) at 0xfffffff2
[fffffff2] *pgd=80000800207003, *pmd=82ffda003, *pte=00000000
Internal error: : 221 [#1] SMP ARM
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc7 #1 NONE
Hardware name: Keystone
PC is at knav_dma_close_channel+0x30/0x19c
LR is at netcp_free_navigator_resources+0x2c/0x28c
[... TRIM...]
Call trace:
knav_dma_close_channel from netcp_free_navigator_resources+0x2c/0x28c
netcp_free_navigator_resources from netcp_ndo_open+0x430/0x46c
netcp_ndo_open from __dev_open+0x114/0x29c
__dev_open from __dev_change_flags+0x190/0x208
__dev_change_flags from netif_change_flags+0x1c/0x58
netif_change_flags from dev_change_flags+0x38/0xa0
dev_change_flags from ip_auto_config+0x2c4/0x11f0
ip_auto_config from do_one_initcall+0x58/0x200
do_one_initcall from kernel_init_freeable+0x1cc/0x238
kernel_init_freeable from kernel_init+0x1c/0x12c
kernel_init from ret_from_fork+0x14/0x38
[... TRIM...]
Standardize the error handling by making the function return NULL on
all error conditions. The API is used in just the netcp_core.c so the
impact is limited.
Note, this change, in effect reverts commit 5b6cb43b4d62 ("net:
ethernet: ti: netcp_core: return error while dma channel open issue"),
but provides a less error prone implementation.
Suggested-by: Simon Horman <horms@kernel.org>
Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Link: https://patch.msgid.link/20251103162811.3730055-1-nm@ti.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
Based on my comprehensive analysis using semantic code analysis tools
and kernel repository examination, here is my determination:
## **YES** - This commit SHOULD be backported to stable kernel trees
---
## Analysis Summary
### Semantic Analysis Tools Used:
1. **mcp__semcode__find_function** - Located all affected functions
2. **mcp__semcode__find_callers** - Identified call sites and impact
scope
3. **mcp__semcode__find_callchain** - Traced execution paths from user-
space
4. **Read** - Examined header file and code structure
5. **Grep** - Verified net_device_ops assignment
6. **git log/show** - Investigated bug history and kernel version
presence
### Key Findings:
#### 1. **User-Space Triggerable Crash** (CRITICAL)
- **Impact**: The bug causes a kernel crash (alignment exception at
0xfffffff2) when opening a network interface
- **Trigger path**: User command (`ifconfig eth0 up` or `ip link set
eth0 up`) → `__dev_open()` → `netcp_ndo_open()`
(drivers/net/ethernet/ti/netcp_core.c:1942) →
`netcp_setup_navigator_resources()` → `knav_dma_open_channel()`
- **Call graph analysis** shows `netcp_ndo_open` is assigned to
`.ndo_open` in `netcp_netdev_ops` structure, making it directly user-
triggerable
#### 2. **Root Cause Analysis**
The crash occurs due to inconsistent error handling:
- **Header stub** (include/linux/soc/ti/knav_dma.h:168): Returns `NULL`
when driver disabled
- **Driver implementation** (drivers/soc/ti/knav_dma.c:407-487): Returns
`(void *)-EINVAL` (ERR_PTR) on errors
- **Callers** check `IS_ERR()` after calling `knav_dma_open_channel()`
- **Cleanup code** in `netcp_free_navigator_resources()` (line 1548)
only checks `if (netcp->rx_channel)` before calling
`knav_dma_close_channel()`
- When `rx_channel` contains `-EINVAL` (0xfffffff2), it's non-NULL, so
the check passes
- `knav_dma_close_channel()` attempts to dereference this invalid
pointer → alignment exception crash
#### 3. **Impact Scope** (from mcp__semcode__find_callers)
- **Limited scope**: Only 2 callers of `knav_dma_open_channel()`:
- `netcp_setup_navigator_resources()`
(drivers/net/ethernet/ti/netcp_core.c:1582)
- `netcp_txpipe_open()` (drivers/net/ethernet/ti/netcp_core.c:1326)
- **Affected hardware**: TI Keystone SoC users with NetCP driver
- **Files changed**: Only 2 files, both in the same subsystem
#### 4. **Bug Longevity**
- **Introduced**: v4.12 (2017) by commit 5b6cb43b4d625
- **Duration**: ~7-8 years of existence
- **Fixed in**: v6.18-rc5 (November 2024)
- Verified the buggy pattern exists in v6.6 LTS kernel
#### 5. **Change Characteristics**
- **Type**: Pure bug fix (crash fix)
- **Size**: Small and contained
(drivers/net/ethernet/ti/netcp_core.c:1326-1366
netcp_core.c:1582-1694, drivers/soc/ti/knav_dma.c:407-487)
- **Semantic changes** (from code inspection):
- Standardizes return value from ERR_PTR to NULL on all error paths
- Updates callers from `IS_ERR()` checks to simple NULL checks
- Updates cleanup from `IS_ERR_OR_NULL()` to simple NULL checks
- Changes error code from `PTR_ERR()` extraction to fixed `-EINVAL`
- **No architectural changes**: No struct modifications, no API
additions
- **No new features**: Only error handling standardization
#### 6. **Backport Risk Assessment**
- **Risk level**: LOW
- Changes are localized to error handling paths
- Makes behavior consistent with header stub
- No complex dependencies identified
- Code structure in v6.6 is compatible (verified)
- **Regression potential**: Minimal
- Affects only error paths that were already broken
- Improves consistency between header and implementation
- All callers updated in same commit
#### 7. **Stable Tree Compliance**
- ✅ **Fixes important bug**: Kernel crash
- ✅ **Small and contained**: 2 files, error handling only
- ✅ **No new features**: Pure bug fix
- ✅ **No architectural changes**: No struct/API changes
- ✅ **Obvious and correct**: Crash trace in commit message proves the
fix
- ✅ **Self-contained**: No dependencies on other commits
- ❌ **Missing stable tags**: No "Cc: stable@vger.kernel.org" or "Fixes:"
tag (oversight)
### Specific Code Reference Points:
1. **drivers/soc/ti/knav_dma.c:407-487**: Changed all error returns from
`(void *)-EINVAL` to `NULL`
2. **drivers/net/ethernet/ti/netcp_core.c:1339**: Changed from
`IS_ERR(tx_pipe->dma_channel)` to `!tx_pipe->dma_channel`
3. **drivers/net/ethernet/ti/netcp_core.c:1361**: Changed from
`IS_ERR_OR_NULL()` to simple NULL check in cleanup
4. **drivers/net/ethernet/ti/netcp_core.c:1680**: Changed from
`IS_ERR(netcp->rx_channel)` to `!netcp->rx_channel`
### Conclusion:
This commit is an **excellent candidate for stable backporting**:
- Fixes a real, user-triggerable kernel crash with documented stack
trace
- Affects users of TI Keystone SoCs who configure network interfaces
- Small, well-contained, low-risk change
- Bug has existed since v4.12 (2017) affecting all stable trees
- Should be backported to all active stable kernels (at minimum v6.1+,
v6.6+, v6.12+)
The absence of stable/Fixes tags appears to be an oversight and should
not prevent backporting given the clear evidence of crash and the
straightforward nature of the fix.
drivers/net/ethernet/ti/netcp_core.c | 10 +++++-----
drivers/soc/ti/knav_dma.c | 14 +++++++-------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 857820657bac5..5ee13db568f08 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1338,10 +1338,10 @@ int netcp_txpipe_open(struct netcp_tx_pipe *tx_pipe)
tx_pipe->dma_channel = knav_dma_open_channel(dev,
tx_pipe->dma_chan_name, &config);
- if (IS_ERR(tx_pipe->dma_channel)) {
+ if (!tx_pipe->dma_channel) {
dev_err(dev, "failed opening tx chan(%s)\n",
tx_pipe->dma_chan_name);
- ret = PTR_ERR(tx_pipe->dma_channel);
+ ret = -EINVAL;
goto err;
}
@@ -1359,7 +1359,7 @@ int netcp_txpipe_open(struct netcp_tx_pipe *tx_pipe)
return 0;
err:
- if (!IS_ERR_OR_NULL(tx_pipe->dma_channel))
+ if (tx_pipe->dma_channel)
knav_dma_close_channel(tx_pipe->dma_channel);
tx_pipe->dma_channel = NULL;
return ret;
@@ -1678,10 +1678,10 @@ static int netcp_setup_navigator_resources(struct net_device *ndev)
netcp->rx_channel = knav_dma_open_channel(netcp->netcp_device->device,
netcp->dma_chan_name, &config);
- if (IS_ERR(netcp->rx_channel)) {
+ if (!netcp->rx_channel) {
dev_err(netcp->ndev_dev, "failed opening rx chan(%s\n",
netcp->dma_chan_name);
- ret = PTR_ERR(netcp->rx_channel);
+ ret = -EINVAL;
goto fail;
}
diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
index a25ebe6cd5030..553ae7ee20f16 100644
--- a/drivers/soc/ti/knav_dma.c
+++ b/drivers/soc/ti/knav_dma.c
@@ -402,7 +402,7 @@ static int of_channel_match_helper(struct device_node *np, const char *name,
* @name: slave channel name
* @config: dma configuration parameters
*
- * Returns pointer to appropriate DMA channel on success or error.
+ * Return: Pointer to appropriate DMA channel on success or NULL on error.
*/
void *knav_dma_open_channel(struct device *dev, const char *name,
struct knav_dma_cfg *config)
@@ -414,13 +414,13 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
if (!kdev) {
pr_err("keystone-navigator-dma driver not registered\n");
- return (void *)-EINVAL;
+ return NULL;
}
chan_num = of_channel_match_helper(dev->of_node, name, &instance);
if (chan_num < 0) {
dev_err(kdev->dev, "No DMA instance with name %s\n", name);
- return (void *)-EINVAL;
+ return NULL;
}
dev_dbg(kdev->dev, "initializing %s channel %d from DMA %s\n",
@@ -431,7 +431,7 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
if (config->direction != DMA_MEM_TO_DEV &&
config->direction != DMA_DEV_TO_MEM) {
dev_err(kdev->dev, "bad direction\n");
- return (void *)-EINVAL;
+ return NULL;
}
/* Look for correct dma instance */
@@ -443,7 +443,7 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
}
if (!dma) {
dev_err(kdev->dev, "No DMA instance with name %s\n", instance);
- return (void *)-EINVAL;
+ return NULL;
}
/* Look for correct dma channel from dma instance */
@@ -463,14 +463,14 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
if (!chan) {
dev_err(kdev->dev, "channel %d is not in DMA %s\n",
chan_num, instance);
- return (void *)-EINVAL;
+ return NULL;
}
if (atomic_read(&chan->ref_count) >= 1) {
if (!check_config(chan, config)) {
dev_err(kdev->dev, "channel %d config miss-match\n",
chan_num);
- return (void *)-EINVAL;
+ return NULL;
}
}
--
2.51.0
^ permalink raw reply related [flat|nested] only message in thread