netdev - Re: [PATCH RESEND net-next v4 4/4] sfc: remove expired unicast PTP filters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fcd12738-5e59-3483-540a-b0f6bb639bbd@gmail.com>
Date:   Tue, 14 Mar 2023 17:24:32 +0000
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Íñigo Huguet <ihuguet@...hat.com>,
        habetsm.xilinx@...il.com, richardcochran@...il.com
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org,
        Yalin Li <yalli@...hat.com>
Subject: Re: [PATCH RESEND net-next v4 4/4] sfc: remove expired unicast PTP
 filters

On 14/03/2023 10:09, Íñigo Huguet wrote:
> Filters inserted to support unicast PTP mode might become unused after
> some time, so we need to remove them to avoid accumulating many of them.
> 
> Actually, it would be a very unusual situation that many different
> addresses are used, normally only a small set of predefined
> addresses are tried. Anyway, some cleanup is necessary because
> maintaining old filters forever makes very little sense.
> 
> Reported-by: Yalin Li <yalli@...hat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@...hat.com>

Acked-by: Edward Cree <ecree.xilinx@...il.com>
but again a couple of nits to think about...

>  static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>  {
> +	struct efx_ptp_data *ptp = efx->ptp_data;
>  	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
>  	struct efx_filter_spec spec;
>  
> @@ -1418,7 +1437,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>  	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
>  	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
>  	spec.ether_type = htons(ETH_P_1588);
> -	return efx_ptp_insert_filter(efx, &efx->ptp_data->rxfilters_mcast, &spec);
> +	return efx_ptp_insert_filter(efx, &ptp->rxfilters_mcast, &spec, 0);
>  }

If respinning for any reason, maybe move the addition of the
 local to patch #2.

> +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx)
> +{
> +	struct efx_ptp_data *ptp = efx->ptp_data;
> +	struct efx_ptp_rxfilter *rxfilter, *tmp;
> +
> +	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) {
> +		if (time_is_before_jiffies(rxfilter->expiry))
> +			efx_ptp_remove_one_filter(efx, rxfilter);
> +	}
> +}
> +
>  static int efx_ptp_start(struct efx_nic *efx)
>  {
>  	struct efx_ptp_data *ptp = efx->ptp_data;
> @@ -1631,6 +1666,8 @@ static void efx_ptp_worker(struct work_struct *work)
>  
>  	while ((skb = __skb_dequeue(&tempq)))
>  		efx_ptp_process_rx(efx, skb);
> +
> +	efx_ptp_drop_expired_unicast_filters(efx);
>  }
PTP worker runs on every PTP packet TX or RX, which might be
 quite frequent.  It's probably fine but do we need to consider
 limiting how much time we spend repeatedly scanning the list?
Conversely, if all PTP traffic suddenly stops, I think existing
 unicast filters will stay indefinitely.  Again probably fine
 but just want to check that sounds sane to everyone.