| 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
| ||
|
Date: Fri, 13 Nov 2020 02:51:24 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Christian Eggers <ceggers@...i.de>
Cc: Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
Richard Cochran <richardcochran@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Vivien Didelot <vivien.didelot@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Kurt Kanzenbach <kurt.kanzenbach@...utronix.de>,
George McCollister <george.mccollister@...il.com>,
Marek Vasut <marex@...x.de>,
Helmut Grohne <helmut.grohne@...enta.de>,
Paul Barker <pbarker@...sulko.com>,
Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>,
Tristram Ha <Tristram.Ha@...rochip.com>,
Woojung Huh <woojung.huh@...rochip.com>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 08/11] net: ptp: add helper for one-step P2P
clocks
On Thu, Nov 12, 2020 at 04:35:34PM +0100, Christian Eggers wrote:
> This function subtracts the ingress hardware time stamp from the
> correction field of a PTP header and updates the UDP checksum (if UDP is
> used as transport. It is needed for hardware capable of one-step P2P
Please close the parenthesis.
> that does not already modify the correction field of Pdelay_Req event
> messages on ingress.
>
> Signed-off-by: Christian Eggers <ceggers@...i.de>
Please add more verbiage here, giving the reader as much context as
possible. You are establishing a de-facto approach for one-step peer
delay timestamping for hardware like yours, you need to explain why it
is done this way, for people to understand just by looking at git blame.
> ---
> include/linux/ptp_classify.h | 97 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
>
> diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
> index 56b2d7d66177..f27b512e1abd 100644
> --- a/include/linux/ptp_classify.h
> +++ b/include/linux/ptp_classify.h
> @@ -10,8 +10,12 @@
> #ifndef _PTP_CLASSIFY_H_
> #define _PTP_CLASSIFY_H_
>
> +#include <asm/unaligned.h>
> #include <linux/ip.h>
> +#include <linux/ktime.h>
> #include <linux/skbuff.h>
> +#include <linux/udp.h>
> +#include <net/checksum.h>
>
> #define PTP_CLASS_NONE 0x00 /* not a PTP event message */
> #define PTP_CLASS_V1 0x01 /* protocol version 1 */
> @@ -118,6 +122,91 @@ static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
> return msgtype;
> }
>
> +/**
> + * ptp_check_diff8 - Computes new checksum (when altering a 64-bit field)
> + * @old: old field value
> + * @new: new field value
> + * @oldsum: previous checksum
> + *
> + * This function can be used to calculate a new checksum when only a single
> + * field is changed. Similar as ip_vs_check_diff*() in ip_vs.h.
> + *
> + * Return: Updated checksum
> + */
> +static inline __wsum ptp_check_diff8(__be64 old, __be64 new, __wsum oldsum)
> +{
> + __be64 diff[2] = { ~old, new };
> +
> + return csum_partial(diff, sizeof(diff), oldsum);
> +}
> +
> +/**
> + * ptp_onestep_p2p_move_t2_to_correction - Update PTP header's correction field
> + * @skb: packet buffer
> + * @type: type of the packet (see ptp_classify_raw())
> + * @hdr: ptp header
> + * @t2: ingress hardware time stamp
> + *
> + * This function subtracts the ingress hardware time stamp from the correction
> + * field of a PTP header and updates the UDP checksum (if UDP is used as
> + * transport). It is needed for hardware capable of one-step P2P that does not
> + * already modify the correction field of Pdelay_Req event messages on ingress.
> + */
> +static inline
> +void ptp_onestep_p2p_move_t2_to_correction(struct sk_buff *skb,
> + unsigned int type,
> + struct ptp_header *hdr,
> + ktime_t t2)
The way this function is coded up right now, there's no reason to call it:
- onestep
- p2p
- move_t2_to_correction
As it is, it would be better named ptp_header_update_correction.
> +{
> + u8 *ptr = skb_mac_header(skb);
> + struct udphdr *uhdr = NULL;
> + s64 ns = ktime_to_ns(t2);
> + __be64 correction_old;
> + s64 correction;
> +
> + /* previous correction value is required for checksum update. */
> + memcpy(&correction_old, &hdr->correction, sizeof(correction_old));
> + correction = (s64)be64_to_cpu(correction_old);
> +
> + /* PTP correction field consists of 32 bit nanoseconds and 16 bit
48 bit nanoseconds
> + * fractional nanoseconds. Avoid shifting negative numbers.
> + */
> + if (ns >= 0)
> + correction -= ns << 16;
> + else
> + correction += -ns << 16;
Again? Why? There's nothing wrong with left-shifting negative numbers,
two's complement works the same (note that right-shifting is a different
story, but that's not the case here).
> +
> + /* write new correction value */
> + put_unaligned_be64((u64)correction, &hdr->correction);
> +
> + /* locate udp header */
> + if (type & PTP_CLASS_VLAN)
> + ptr += VLAN_HLEN;
Can't you just go back from the struct ptp_header pointer and avoid this
check?
> + ptr += ETH_HLEN;
> +
> + switch (type & PTP_CLASS_PMASK) {
> + case PTP_CLASS_IPV4:
> + ptr += ((struct iphdr *)ptr)->ihl << 2;
> + uhdr = (struct udphdr *)ptr;
> + break;
> + case PTP_CLASS_IPV6:
> + ptr += IP6_HLEN;
> + uhdr = (struct udphdr *)ptr;
> + break;
> + }
> +
> + if (!uhdr)
> + return;
> +
> + /* update checksum */
> + uhdr->check = csum_fold(ptp_check_diff8(correction_old,
> + hdr->correction,
> + ~csum_unfold(uhdr->check)));
> + if (!uhdr->check)
> + uhdr->check = CSUM_MANGLED_0;
> +}
> +
>
Powered by blists - more mailing lists