| 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: <BY3PR18MB4707FCB6E39FA7E6DBBDFFA0A0AA9@BY3PR18MB4707.namprd18.prod.outlook.com>
Date: Wed, 22 Feb 2023 09:46:02 +0000
From: Sai Krishna Gajula <saikrishnag@...vell.com>
To: Paolo Abeni <pabeni@...hat.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sunil Kovvuri Goutham <sgoutham@...vell.com>,
Geethasowjanya Akula <gakula@...vell.com>,
"richardcochran@...il.com" <richardcochran@...il.com>
CC: Hariprasad Kelam <hkelam@...vell.com>
Subject: Re: [net PATCH] octeontx2-pf: Recalculate UDP checksum for ptp 1-step
sync packet
Hi Paolo,
Please see the responses inline.
> -----Original Message-----
> From: Paolo Abeni <pabeni@...hat.com>
> Sent: Tuesday, February 21, 2023 5:10 PM
> To: Sai Krishna Gajula <saikrishnag@...vell.com>; davem@...emloft.net;
> edumazet@...gle.com; kuba@...nel.org; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; Sunil Kovvuri Goutham <sgoutham@...vell.com>;
> Geethasowjanya Akula <gakula@...vell.com>; richardcochran@...il.com
> Cc: Hariprasad Kelam <hkelam@...vell.com>
> Subject: Re: [net PATCH] octeontx2-pf: Recalculate UDP checksum for
> ptp 1-step sync packet
>
> On Mon, 2023-02-20 at 17:50 +0530, Sai Krishna wrote:
> > From: Geetha sowjanya <gakula@...vell.com>
> >
> > When checksum offload is disabled in the driver via ethtool, the PTP
> > 1-step sync packets contain incorrect checksum, since the stack
> > calculates the checksum before driver updates PTP timestamp field in
> > the packet. This results in PTP packets getting dropped at the other
> > end. This patch fixes the issue by re-calculating the UDP checksum
> > after updating PTP timestamp field in the driver.
> >
> > Fixes: 2958d17a8984 ("octeontx2-pf: Add support for ptp 1-step mode on
> > CN10K silicon")
> > Signed-off-by: Geetha sowjanya <gakula@...vell.com>
> > Signed-off-by: Hariprasad Kelam <hkelam@...vell.com>
> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@...vell.com>
> > Signed-off-by: Sai Krishna <saikrishnag@...vell.com>
> > ---
> > .../marvell/octeontx2/nic/otx2_txrx.c | 78 ++++++++++++++-----
> > 1 file changed, 59 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > index ef10aef3cda0..67345a3e2bba 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > @@ -10,6 +10,7 @@
> > #include <net/tso.h>
> > #include <linux/bpf.h>
> > #include <linux/bpf_trace.h>
> > +#include <net/ip6_checksum.h>
> >
> > #include "otx2_reg.h"
> > #include "otx2_common.h"
> > @@ -699,7 +700,7 @@ static void otx2_sqe_add_ext(struct otx2_nic
> > *pfvf, struct otx2_snd_queue *sq,
> >
> > static void otx2_sqe_add_mem(struct otx2_snd_queue *sq, int *offset,
> > int alg, u64 iova, int ptp_offset,
> > - u64 base_ns, int udp_csum)
> > + u64 base_ns, bool udp_csum_crt)
> > {
> > struct nix_sqe_mem_s *mem;
> >
> > @@ -711,7 +712,7 @@ static void otx2_sqe_add_mem(struct
> otx2_snd_queue
> > *sq, int *offset,
> >
> > if (ptp_offset) {
> > mem->start_offset = ptp_offset;
> > - mem->udp_csum_crt = udp_csum;
> > + mem->udp_csum_crt = !!udp_csum_crt;
> > mem->base_ns = base_ns;
> > mem->step_type = 1;
> > }
> > @@ -986,10 +987,11 @@ static bool
> otx2_validate_network_transport(struct sk_buff *skb)
> > return false;
> > }
> >
> > -static bool otx2_ptp_is_sync(struct sk_buff *skb, int *offset, int
> > *udp_csum)
> > +static bool otx2_ptp_is_sync(struct sk_buff *skb, int *offset, bool
> > +*udp_csum_crt)
> > {
> > struct ethhdr *eth = (struct ethhdr *)(skb->data);
> > u16 nix_offload_hlen = 0, inner_vhlen = 0;
> > + bool udp_hdr_present = false, is_sync;
> > u8 *data = skb->data, *msgtype;
> > __be16 proto = eth->h_proto;
> > int network_depth = 0;
> > @@ -1029,45 +1031,83 @@ static bool otx2_ptp_is_sync(struct sk_buff
> *skb, int *offset, int *udp_csum)
> > if (!otx2_validate_network_transport(skb))
> > return false;
> >
> > - *udp_csum = 1;
> > *offset = nix_offload_hlen + skb_transport_offset(skb) +
> > sizeof(struct udphdr);
> > + udp_hdr_present = true;
> > +
> > }
> >
> > msgtype = data + *offset;
> > -
> > /* Check PTP messageId is SYNC or not */
> > - return (*msgtype & 0xf) == 0;
> > + is_sync = ((*msgtype & 0xf) == 0) ? true : false;
>
> the above could be:
>
> is_sync = !(*msgtype & 0xf);
>
I will make changes and submit v2 patch.
> possibly more readable.
>
> > + if (is_sync) {
> > + if (udp_hdr_present)
> > + *udp_csum_crt = true;
>
> or:
> *udp_csum_crt = udp_hdr_present;
>
> that will make the code more compact.
>
I will make changes and submit v2 patch.
> > + } else {
> > + *offset = 0;
> > + }
> > +
> > + return is_sync;
> > }
> >
> > static void otx2_set_txtstamp(struct otx2_nic *pfvf, struct sk_buff *skb,
> > struct otx2_snd_queue *sq, int *offset) {
> > + struct ethhdr *eth = (struct ethhdr *)(skb->data);
> > struct ptpv2_tstamp *origin_tstamp;
> > - int ptp_offset = 0, udp_csum = 0;
> > + bool udp_csum_crt = false;
> > + unsigned int udphoff;
> > struct timespec64 ts;
> > + int ptp_offset = 0;
> > + __wsum skb_csum;
> > u64 iova;
> >
> > if (unlikely(!skb_shinfo(skb)->gso_size &&
> > (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))) {
> > - if (unlikely(pfvf->flags & OTX2_FLAG_PTP_ONESTEP_SYNC)) {
> > - if (otx2_ptp_is_sync(skb, &ptp_offset, &udp_csum)) {
> > - origin_tstamp = (struct ptpv2_tstamp *)
> > - ((u8 *)skb->data + ptp_offset
> +
> > - PTP_SYNC_SEC_OFFSET);
> > - ts = ns_to_timespec64(pfvf->ptp->tstamp);
> > - origin_tstamp->seconds_msb =
> htons((ts.tv_sec >> 32) & 0xffff);
> > - origin_tstamp->seconds_lsb = htonl(ts.tv_sec
> & 0xffffffff);
> > - origin_tstamp->nanoseconds =
> htonl(ts.tv_nsec);
> > - /* Point to correction field in PTP packet */
> > - ptp_offset += 8;
> > + if (unlikely(pfvf->flags & OTX2_FLAG_PTP_ONESTEP_SYNC &&
> > + otx2_ptp_is_sync(skb, &ptp_offset,
> &udp_csum_crt))) {
> > + origin_tstamp = (struct ptpv2_tstamp *)
> > + ((u8 *)skb->data + ptp_offset +
> > + PTP_SYNC_SEC_OFFSET);
> > + ts = ns_to_timespec64(pfvf->ptp->tstamp);
> > + origin_tstamp->seconds_msb = htons((ts.tv_sec >>
> 32) & 0xffff);
> > + origin_tstamp->seconds_lsb = htonl(ts.tv_sec &
> 0xffffffff);
> > + origin_tstamp->nanoseconds = htonl(ts.tv_nsec);
> > + /* Point to correction field in PTP packet */
> > + ptp_offset += 8;
> > +
> > + /* When user disables hw checksum, stack calculates
> the csum,
> > + * but it does not cover ptp timestamp which is added
> later.
> > + * Recalculate the checksum manually considering the
> timestamp.
> > + */
> > + if (udp_csum_crt) {
> > + struct udphdr *uh = udp_hdr(skb);
> > +
> > + if (skb->ip_summed != CHECKSUM_PARTIAL
> && uh->check != 0) {
> > + udphoff = skb_transport_offset(skb);
> > + uh->check = 0;
> > + skb_csum = skb_checksum(skb,
> udphoff, skb->len - udphoff,
> > + 0);
> > + if (ntohs(eth->h_proto) ==
> ETH_P_IPV6)
> > + uh->check =
> csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
> > +
> &ipv6_hdr(skb)->daddr,
> > +
> skb->len - udphoff,
> > +
> ipv6_hdr(skb)->nexthdr,
> > +
> skb_csum);
> > + else
> > + uh->check =
> csum_tcpudp_magic(ip_hdr(skb)->saddr,
> > +
> ip_hdr(skb)->daddr,
> > +
> skb->len - udphoff,
> > +
> IPPROTO_UDP,
> > +
> skb_csum);
>
> Have you considered incremental csum updates instead? Possibly the code
> could be simpler and likely faster - not sure if the latter part is relevant in this
> case.
>
We don't expect any significant performance improvement with this for PTP sync packets.
So we didn't consider incremental csum updates.
Thanks,
Sai
> Cheers,
>
> Paolo
Powered by blists - more mailing lists