Re: [PATCH net-next v3 11/12] net: atlantic: add AQC113 TX timestamp polling and PTP TX classification

[Jakub Kicinski <kuba@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.
---
net: atlantic: add AQC113 TX timestamp polling and PTP TX classification

This commit adds PTP TX classification for IPv4 and IPv6 packets. It also
introduces a polling mechanism for hardware TX timestamps in the driver's
ring cleanup path.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index 4ef4fe64b8ac1..1da14786fe5cc 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
[ ... ]
> @@ -68,14 +70,6 @@ int aq_ndev_open(struct net_device *ndev)
>  	if (err < 0)
>  		goto err_exit;
>  
> -	err = aq_reapply_rxnfc_all_rules(aq_nic);
> -	if (err < 0)
> -		goto err_exit;
> -
> -	err = aq_filters_vlans_update(aq_nic);
> -	if (err < 0)
> -		goto err_exit;

[Severity: High]
The commit message mentions removing a stale PTP call, but doesn't this
remove the restoration of RX flow rules and VLAN filters when the device is
brought up? It doesn't look like these were moved to aq_nic_start().

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e270327e47fd8..87d680590ef2f 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
[ ... ]
> @@ -311,6 +311,26 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
>  		if (likely(!buff->is_eop))
>  			goto out;
>  
> +		if (unlikely(buff->request_ts) &&
> +		    self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts) {
> +			u64 ts = self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts(self);
> +
> +			if (!ts) {
> +				if (time_after(jiffies,
> +					       self->ptp_ts_deadline)) {

[Severity: High]
Where is self->ptp_ts_deadline initialized?

If it is never initialized and defaults to 0, will this condition always
evaluate to true on the very first poll since jiffies is typically greater
than 0, causing all PTP packets to immediately time out and be dropped?

> +					/* Timeout: free skb, unblock ring */
> +					buff->request_ts = 0;
> +					dev_kfree_skb_any(buff->skb);
> +					buff->skb = NULL;

[Severity: High]
If a timeout occurs, buff->skb is freed and set to NULL, but there is no
break or continue here. Doesn't execution then fall through to calling
aq_ptp_tx_hwtstamp() while the SKB is already freed, leading to a
use-after-free when the PTP logic attempts to access the SKB to report
the timeout?

Additionally, since buff->skb is now NULL, won't this skip the subsequent
transmission statistics update block?

> +				} else {
> +					buff->is_mapped = 0;
> +					buff->pa = 0U;
> +					break;
> +				}
> +			}
> +
> +			aq_ptp_tx_hwtstamp(self->aq_nic, ts);
> +		}