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: <a2d55af3-df0c-2ed8-fc2c-78c0da6e6390@redhat.com>
Date:   Wed, 15 Mar 2023 09:54:35 +0100
From:   Íñigo Huguet <ihuguet@...hat.com>
To:     Edward Cree <ecree.xilinx@...il.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



El 14/3/23 a las 18:24, Edward Cree escribió:
> 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?

PTP traffic is not that frequent, few packets per second, isn't it?

> 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.

Yes, it's as you say. However, I thought it didn't worth it to create a new periodic worker only for this, given that I expected a short list, it wouldn't be harmful. However, as I said in the other message, maybe the list can be quite long if we're the PTP master?

Maybe I should create a dedicated periodic work for this? That would avoid both problems that you are pointing out.