Re: [PATCH net] nfc: clear active_target when the target list is replaced

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
nfc: clear active_target when the target list is replaced

This patch updates nfc_targets_found() so that when the active target
points into the dev->targets array about to be freed, the cached pointer
is cleared and a partial teardown (presence-check timer stop, dep_link_up
cleared, rf_mode reset) is performed before kfree(dev->targets).

> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index a92a6566e6a0..950807906645 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -786,6 +786,21 @@ int nfc_targets_found(struct nfc_dev *dev,
>  
>  	dev->targets_generation++;
>  
> +	if (dev->active_target && dev->targets) {
> +		for (i = 0; i < dev->n_targets; i++) {
> +			if (dev->active_target != &dev->targets[i])
> +				continue;
> +
> +			if (dev->ops->check_presence)
> +				timer_delete_sync(&dev->check_pres_timer);
> +
> +			dev->active_target = NULL;
> +			dev->dep_link_up = false;
> +			dev->rf_mode = NFC_RF_NONE;
> +			break;
> +		}
> +	}
> +

[Medium]
The commit message says this teardown mirrors what nfc_deactivate_target()
does, but nfc_deactivate_target() does not touch dev->dep_link_up at all.
Forcing dep_link_up = false here when the active target was set up via
nfc_dep_link_up() introduces a state mutation that is not described in
the commit log.

Can this leave DEP/LLCP state only partially torn down?

Compared to nfc_dep_link_down(), this new branch skips:

	rc = dev->ops->dep_link_down(dev);
	...
	nfc_llcp_mac_is_down(dev);
	nfc_genl_dep_link_down_event(dev);

so the driver is never told to bring the DEP link down, LLCP sockets
bound to the local are not released via nfc_llcp_socket_release(), and
no NFC_EVENT_DEP_LINK_DOWN is sent to userspace.

A subsequent userspace nfc_dep_link_down() will then be rejected here:

net/nfc/core.c:nfc_dep_link_down() {
	...
	if (dev->dep_link_up == false) {
		rc = -EALREADY;
		goto error;
	}
	...
}

so the normal cleanup path that calls nfc_llcp_mac_is_down() is no
longer reachable from userspace until device unregister.

Does this risk leaking the LLCP sockets that were created while
dep_link_up was true?

If the goal is just to clear the dangling active_target pointer, would
it be enough to clear dev->active_target (and stop the presence-check
timer) without also forcing dep_link_up = false and rf_mode = NFC_RF_NONE?
Alternatively, if a full DEP teardown is intended, should this path call
dev->ops->dep_link_down(dev) and nfc_llcp_mac_is_down(dev) like
nfc_dep_link_down() does?