Re: [RFC net-next 4/4] ptp: ocp: Add R4006 I2C peripheral topology

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

New issues:
- [Medium] Repeated failures in firmware node registration inside `ptp_ocp_=
r4006_init_nodes` will leak `devm`-allocated memory because the function is=
 called from a retriable workqueue.

Pre-existing issues:
- [High] Lockless access to `dev->driver` in the global I2C notifier parent=
 tree traversal creates a TOCTOU race condition leading to a NULL pointer d=
ereference.
--

--- Patch [4]: [RFC net-next 4/4] ptp: ocp: Add R4006 I2C peripheral topolo=
gy ---
commit 6482a6be1fc3b33821906c9b60253b2b40290bb4
Author: Ahmad Byagowi <[email protected]>

    ptp: ocp: Add R4006 I2C peripheral topology

This commit adds support for the R4006 I2C peripheral topology, detecting
the board via EEPROM and describing the topology with software nodes. It
registers standard I2C clients upon adapter appearance and handles I2C
bus notifications for dynamic population.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 35e911f1ad78c..b362a7c4dbb5f 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
[ ... ]
> +static int
> +ptp_ocp_r4006_init_nodes(struct ptp_ocp *bp)
> +{
[ ... ]
> +	r4006 =3D devm_kzalloc(dev, sizeof(*r4006), GFP_KERNEL);
> +	if (!r4006)
> +		return -ENOMEM;
> +
> +	r4006->mux_node.name =3D
> +		devm_kasprintf(dev, GFP_KERNEL, "ocp%d-r4006-mux", bp->id);
[ ... ]
> +	ret =3D software_node_register_node_group(r4006->node_group);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register R4006 firmware nodes\n");
> +
> +	r4006->registered =3D true;
> +	bp->r4006 =3D r4006;
> +	return 0;
> +}

[Severity: Medium]
Will this leak devm-allocated memory on failure?=20

Since ptp_ocp_r4006_init_nodes() is called from ptp_ocp_i2c_work() (which
can be dynamically re-triggered by I2C bus notifications), any deterministic
failure in software_node_register_node_group() will cause an early return
without freeing the memory allocated via devm_kzalloc() and
devm_kasprintf() earlier in this function. Could this repeatedly leak
memory on the device's devres list with each worker execution?

[ ... ]
> +static struct ptp_ocp *
> +ptp_ocp_i2c_parent(struct device *child)
> +{
> +	struct device *dev =3D child;
> +
> +	while ((dev =3D dev->parent))
> +		if (dev->driver && !strcmp(dev->driver->name, KBUILD_MODNAME))
> +			return dev_get_drvdata(dev);
> +
> +	return NULL;
> +}

[Severity: High]
This is a pre-existing issue, but does this lockless check of dev->driver
risk a NULL pointer dereference?

If a parent device is concurrently unbinding while this global I2C notifier
is traversing the tree, the driver core clears its dev->driver pointer.
If dev->driver is evaluated as non-NULL but becomes NULL right before
strcmp() accesses dev->driver->name, it could crash the kernel.

Should we be taking the device_lock() before evaluating dev->driver?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/cover.1785684064.gi=
[email protected]?part=3D4