Re: [PATCH net-next v7 05/15] net: ethernet: oa_tc6: Move oa_tc6.c to its own directory

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] The patch renames the resulting kernel module from `oa_tc6.ko` to `oa_tc6_mod.ko`, which can break existing userspace configurations.
- [Low] The Kconfig prompt for `OA_TC6` drops the `if COMPILE_TEST` condition, making an internal library unconditionally visible in menuconfig.
- [Low] The Makefile uses the `:=` assignment operator for `obj-$(CONFIG_OA_TC6)` instead of the standard `+=` operator.
- [Low] The new MAINTAINERS wildcard pattern fails to cover the Makefile, Kconfig, and future files in the new directory.

Pre-existing issues:
- [High] Missing bounds check before `skb_put()` leads to `skb_over_panic` if the MAC-PHY sends an overly large frame.
- [High] Spurious frame chunks without a preceding start chunk cause a NULL pointer dereference.
- [High] Receiving multiple frame start chunks without end chunks leaks SKBs.
--

--- Patch [5]: [PATCH net-next v7 05/15] net: ethernet: oa_tc6: Move oa_tc6.c to its own directory ---
commit 5ceee1aba13100b21fa64b6c007fc879eb9ec499
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Move oa_tc6.c to its own directory

This commit moves oa_tc6.c to its own directory under drivers/net/ethernet.
This refactoring facilitates adding additional files to support other features
defined by the OPEN Alliance 10BASE-T1x Serial Interface specification, such
as hardware timestamping and PTP-related APIs.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index fab0804329db4..c9b101d8e1fd2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20219,7 +20219,7 @@ M:	Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/networking/oa-tc6-framework.rst
> -F:	drivers/net/ethernet/oa_tc6.c
> +F:	drivers/net/ethernet/oa_tc6/oa_tc6*

[Severity: Low]
Does this file matching pattern miss the new Kconfig, Makefile, and future
files in this directory?

A directory path wildcard F: drivers/net/ethernet/oa_tc6/ would cover all
relevant files in the new location.

[ ... ]

> diff --git a/drivers/net/ethernet/oa_tc6/Kconfig b/drivers/net/ethernet/oa_tc6/Kconfig
> new file mode 100644
> index 0000000000000..97345f345fb9f
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# OA TC6 10BASE-T1x MAC-PHY configuration
> +#
> +
> +config OA_TC6
> +	tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support"

[Severity: Low]
Does dropping the "if COMPILE_TEST" condition unconditionally expose this
internal framework in the kernel configuration menus?

[ ... ]

> diff --git a/drivers/net/ethernet/oa_tc6/Makefile b/drivers/net/ethernet/oa_tc6/Makefile
> new file mode 100644
> index 0000000000000..f24aae852ef27
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for OA TC6 10BASE-T1x MAC-PHY
> +#
> +
> +obj-$(CONFIG_OA_TC6) := oa_tc6_mod.o

[Severity: Low]
Does the use of := instead of += violate the standard Kbuild pattern for
appending objects to obj-$(CONFIG_...)?

Using := overwrites the variable entirely, which may be brittle against future
additions.

[Severity: High]
Does renaming the resulting module from oa_tc6.ko to oa_tc6_mod.ko break
existing userspace configurations?

Renaming a kernel module breaks userspace dependencies, initramfs builds,
and modprobe.d configurations that expect the original name.

To allow multiple source files without renaming the module, the standard
practice is to rename the C file instead (e.g., oa_tc6_core.c) and use
oa_tc6-y := oa_tc6_core.o ptp.o while keeping obj-$(CONFIG_OA_TC6) += oa_tc6.o.

> +oa_tc6_mod-objs := oa_tc6.o

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> similarity index 100%
> rename from drivers/net/ethernet/oa_tc6.c
> rename to drivers/net/ethernet/oa_tc6/oa_tc6.c

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of bounds checking
before calling skb_put() lead to an skb_over_panic if the MAC-PHY sends an
overly large frame?

If a MAC-PHY sends a continuous stream of ongoing chunks without an end_valid
bit, the driver repeatedly appends 64-byte payloads via oa_tc6_update_rx_skb():

drivers/net/ethernet/oa_tc6/oa_tc6.c:oa_tc6_update_rx_skb() {
    memcpy(skb_put(tc6->rx_skb, length), payload, length);
}

The SKB is allocated with a fixed maximum size (MTU + headers).

When the tailroom is exhausted, skb_put() triggers a kernel panic.

[Severity: High]
This isn't a bug introduced by this patch, but do spurious frame chunks without
a preceding start chunk cause a NULL pointer dereference?

If the MAC-PHY sends an ongoing chunk (start_valid=0, end_valid=0) or an end
chunk without a preceding valid start chunk, tc6->rx_skb will be NULL.

The driver directly passes these chunks to oa_tc6_update_rx_skb(), which calls
skb_put(tc6->rx_skb, length).

Dereferencing the NULL pointer inside skb_put() will crash the kernel.

[Severity: High]
This isn't a bug introduced by this patch, but does receiving multiple frame
start chunks without end chunks leak SKBs?

In oa_tc6_prcs_rx_chunk_payload(), if the driver receives a chunk with
start_valid=1 while tc6->rx_skb is already allocated, oa_tc6_prcs_rx_frame_start()
is called.

This unconditionally calls oa_tc6_allocate_rx_skb():

drivers/net/ethernet/oa_tc6/oa_tc6.c:oa_tc6_allocate_rx_skb() {
    tc6->rx_skb = netdev_alloc_skb_ip_align(tc6->netdev, tc6->netdev->mtu +
                                            ETH_HLEN + ETH_FCS_LEN);
    ...
}

This overwrites the tc6->rx_skb pointer. The previously allocated SKB is never
freed, resulting in a memory leak that can cause OOM.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-s2500-mac-phy-support-v7-0-478c877aa1a9@onsemi.com?part=5