| 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
| ||
|
Message-ID: <CACT4ouc-AvwPTN5pf5MCZ8-xYmMPsQpozQfszda0Cu0SuHkrfw@mail.gmail.com>
Date: Fri, 3 Feb 2023 16:18:51 +0100
From: Íñigo Huguet <ihuguet@...hat.com>
To: Íñigo Huguet <ihuguet@...hat.com>,
ecree.xilinx@...il.com, richardcochran@...il.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, netdev@...r.kernel.org,
Yalin Li <yalli@...hat.com>
Subject: Re: [PATCH net-next v2 4/4] sfc: remove expired unicast PTP filters
On Thu, Feb 2, 2023 at 3:12 PM Martin Habets <habetsm.xilinx@...il.com> wrote:
>
> On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñ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>
> > ---
> > drivers/net/ethernet/sfc/ptp.c | 121 +++++++++++++++++++++------------
> > 1 file changed, 77 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> > index a3e827cd84a8..dd46ca6c070e 100644
> > --- a/drivers/net/ethernet/sfc/ptp.c
> > +++ b/drivers/net/ethernet/sfc/ptp.c
> > @@ -75,6 +75,9 @@
> > /* How long an unmatched event or packet can be held */
> > #define PKT_EVENT_LIFETIME_MS 10
> >
> > +/* How long unused unicast filters can be held */
> > +#define UCAST_FILTER_EXPIRY_JIFFIES msecs_to_jiffies(30000)
>
> This seems like something that should be tunable, with this as a
> default value.
>
> > +
> > /* Offsets into PTP packet for identification. These offsets are from the
> > * start of the IP header, not the MAC header. Note that neither PTP V1 nor
> > * PTP V2 permit the use of IPV4 options.
> > @@ -220,6 +223,7 @@ struct efx_ptp_timeset {
> > * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6)
> > * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT)
> > * @loc_host: IPv4/v6 address of the filter
> > + * @expiry: time when the filter expires, in jiffies
> > * @handle: Handle ID for the MCDI filters table
> > */
> > struct efx_ptp_rxfilter {
> > @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter {
> > __be16 ether_type;
> > __be16 loc_port;
> > __be32 loc_host[4];
> > + unsigned long expiry;
> > int handle;
> > };
> >
> > @@ -1320,8 +1325,8 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
> > local_bh_enable();
> > }
> >
> > -static bool efx_ptp_filter_exists(struct list_head *ptp_list,
> > - struct efx_filter_spec *spec)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec)
> > {
> > struct efx_ptp_rxfilter *rxfilter;
> >
> > @@ -1329,10 +1334,19 @@ static bool efx_ptp_filter_exists(struct list_head *ptp_list,
> > if (rxfilter->ether_type == spec->ether_type &&
> > rxfilter->loc_port == spec->loc_port &&
> > !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)))
> > - return true;
> > + return rxfilter;
> > }
> >
> > - return false;
> > + return NULL;
> > +}
> > +
> > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> > + struct efx_ptp_rxfilter *rxfilter)
>
> As others noted, don't use inline in .c files.
>
> > +{
> > + efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
> > + rxfilter->handle);
> > + list_del(&rxfilter->list);
> > + kfree(rxfilter);
> > }
> >
> > static void efx_ptp_remove_filters(struct efx_nic *efx,
> > @@ -1341,10 +1355,7 @@ static void efx_ptp_remove_filters(struct efx_nic *efx,
> > struct efx_ptp_rxfilter *rxfilter, *tmp;
> >
> > list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) {
> > - efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
> > - rxfilter->handle);
> > - list_del(&rxfilter->list);
> > - kfree(rxfilter);
> > + efx_ptp_remove_one_filter(efx, rxfilter);
> > }
> > }
> >
> > @@ -1358,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx,
> > efx_rx_queue_index(queue));
> > }
> >
> > -static int efx_ptp_insert_filter(struct efx_nic *efx,
> > - struct list_head *ptp_list,
> > - struct efx_filter_spec *spec)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > + struct efx_filter_spec *spec)
>
> This API change and the following ones are all for very little gain,
> they are just do set the new expiry attribute. And in the end the
> pointers all get converted back to an integer return code.
> In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter()
> and efx_ptp_insert_ipv6_filter().
I wanted to save computation time when inserting multicast filters,
but given that it's done only once, it's a bit pointless. I also
wanted to show clearly in the code that only unicast filters care
about expiration, but maybe just passing a constant 0 to your expiry
argument clearly shows that multicast doesn't care about it.
I also had the feeling that I was overcomplicating the API for little
gain, I like your approach more.
Also, reviewing this has made me to realize that I'm inserting
pointless unicast filters when using multicast PTP. Probably I should
add a check `ip_hdr(skb)->daddr != htnonl(PTP_ADDR_IPV4)` to
efx_ptp_valid_unicast_event_pkt (and the equivalent for IPv6).
>
> > {
> > struct efx_ptp_rxfilter *rxfilter;
> > int rc;
> >
> > - if (efx_ptp_filter_exists(ptp_list, spec))
> > - return 0;
> > + rxfilter = efx_ptp_find_filter(ptp_list, spec);
> > + if (rxfilter)
> > + return rxfilter;
> >
> > rc = efx_filter_insert_filter(efx, spec, true);
> > if (rc < 0)
> > - return rc;
> > + return ERR_PTR(rc);
> >
> > rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
> > if (!rxfilter)
> > - return -ENOMEM;
> > + return ERR_PTR(-ENOMEM);
> >
> > rxfilter->handle = rc;
> > rxfilter->ether_type = spec->ether_type;
> > @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx,
> > memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
> > list_add(&rxfilter->list, ptp_list);
> >
> > - return 0;
> > + return rxfilter;
> > }
> >
> > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> > - struct list_head *ptp_list,
> > - __be32 addr, u16 port)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > + __be32 addr, u16 port)
> > {
> > struct efx_filter_spec spec;
> >
> > @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
> > return efx_ptp_insert_filter(efx, ptp_list, &spec);
> > }
> >
> > -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> > - struct list_head *ptp_list,
> > - struct in6_addr *addr, u16 port)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list,
> > + struct in6_addr *addr, u16 port)
> > {
> > struct efx_filter_spec spec;
> >
> > @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
> > return efx_ptp_insert_filter(efx, ptp_list, &spec);
> > }
> >
> > -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> > +static struct efx_ptp_rxfilter *
> > +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> > {
> > const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
> > struct efx_filter_spec spec;
> > @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
> > static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> > {
> > struct efx_ptp_data *ptp = efx->ptp_data;
> > - int rc;
> > + struct efx_ptp_rxfilter *rc;
> >
> > if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast))
> > return 0;
> > @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> > */
> > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
> > htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
> > - if (rc < 0)
> > + if (IS_ERR(rc))
> > goto fail;
> >
> > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
> > htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
> > - if (rc < 0)
> > + if (IS_ERR(rc))
> > goto fail;
> >
> > /* if the NIC supports hw timestamps by the MAC, we can support
> > @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >
> > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
> > &ipv6_addr, PTP_EVENT_PORT);
> > - if (rc < 0)
> > + if (IS_ERR(rc))
> > goto fail;
> >
> > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
> > &ipv6_addr, PTP_GENERAL_PORT);
> > - if (rc < 0)
> > + if (IS_ERR(rc))
> > goto fail;
> >
> > rc = efx_ptp_insert_eth_multicast_filter(efx);
> > - if (rc < 0)
> > + if (IS_ERR(rc))
> > goto fail;
> > }
> >
> > @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
> >
> > fail:
> > efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast);
> > - return rc;
> > + return PTR_ERR(rc);
> > }
> >
> > static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb)
> > @@ -1484,7 +1497,7 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> > struct sk_buff *skb)
> > {
> > struct efx_ptp_data *ptp = efx->ptp_data;
> > - int rc;
> > + struct efx_ptp_rxfilter *rxfilter;
> >
> > if (!efx_ptp_valid_unicast_event_pkt(skb))
> > return -EINVAL;
> > @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> > if (skb->protocol == htons(ETH_P_IP)) {
> > __be32 addr = ip_hdr(skb)->saddr;
> >
> > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > - addr, PTP_EVENT_PORT);
> > - if (rc < 0)
> > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > + addr, PTP_EVENT_PORT);
> > + if (IS_ERR(rxfilter))
> > goto fail;
> >
> > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > - addr, PTP_GENERAL_PORT);
> > - if (rc < 0)
> > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > +
> > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
> > + addr, PTP_GENERAL_PORT);
> > + if (IS_ERR(rxfilter))
> > goto fail;
> > +
> > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > } else if (efx_ptp_use_mac_tx_timestamps(efx)) {
> > /* IPv6 PTP only supported by devices with MAC hw timestamp */
> > struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
> >
> > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > - addr, PTP_EVENT_PORT);
> > - if (rc < 0)
> > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > + addr, PTP_EVENT_PORT);
> > + if (IS_ERR(rxfilter))
> > goto fail;
> >
> > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > - addr, PTP_GENERAL_PORT);
> > - if (rc < 0)
> > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > +
> > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
> > + addr, PTP_GENERAL_PORT);
> > + if (IS_ERR(rxfilter))
> > goto fail;
> > +
> > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
> > } else {
> > return -EOPNOTSUPP;
> > }
> > @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
> >
> > fail:
> > efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
> > - return rc;
> > + return PTR_ERR(rxfilter);
> > +}
> > +
> > +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))
>
> Shouldn't this be time_is_after_jiffies?
>
> > + efx_ptp_remove_one_filter(efx, rxfilter);
> > + }
> > }
> >
> > static int efx_ptp_start(struct efx_nic *efx)
> > @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work)
> > }
> >
> > efx_ptp_drop_time_expired_events(efx);
> > + efx_ptp_drop_expired_unicast_filters(efx);
>
> This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because
> that will delay processing that is done below. So do this at the end of
> the function.
>
> Martin
>
> >
> > __skb_queue_head_init(&tempq);
> > efx_ptp_process_events(efx, &tempq);
> > --
> > 2.34.3
>
--
Íñigo Huguet
Powered by blists - more mailing lists