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: <662a69475869_1de39b29415@willemb.c.googlers.com.notmuch>
Date: Thu, 25 Apr 2024 10:31:35 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Abhishek Chauhan <quic_abchauha@...cinc.com>, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 Andrew Halaney <ahalaney@...hat.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Martin KaFai Lau <martin.lau@...nel.org>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 bpf <bpf@...r.kernel.org>
Cc: kernel@...cinc.com
Subject: Re: [RFC PATCH bpf-next v5 1/2] net: Rename mono_delivery_time to
 tstamp_type for scalabilty

Abhishek Chauhan wrote:
> mono_delivery_time was added to check if skb->tstamp has delivery
> time in mono clock base (i.e. EDT) otherwise skb->tstamp has
> timestamp in ingress and delivery_time at egress.
> 
> Renaming the bitfield from mono_delivery_time to tstamp_type is for
> extensibilty for other timestamps such as userspace timestamp
> (i.e. SO_TXTIME) set via sock opts.
> 
> As we are renaming the mono_delivery_time to tstamp_type, it makes
> sense to start assigning tstamp_type based on enum defined
> in this commit.
> 
> Earlier we used bool arg flag to check if the tstamp is mono in
> function skb_set_delivery_time, Now the signature of the functions
> accepts tstamp_type to distinguish between mono and real time.
> 
> Introduce a new function to set tstamp_type based on clockid. 
> 
> In future tstamp_type:1 can be extended to support userspace timestamp
> by increasing the bitfield.
> 
> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
> Signed-off-by: Abhishek Chauhan <quic_abchauha@...cinc.com>

> +static inline void skb_set_tstamp_type_frm_clkid(struct sk_buff *skb,
> +						  ktime_t kt, clockid_t clockid)
> +{

Please don't garble words to save a few characters: .._from_clockid.

And this is essentially skb_set_delivery_type, just taking another
type. So skb_set_delivery_type_(by|from)_clockid.

Also, instead of reimplementing the same logic with a different
type, could implement as a conversion function that calls the main
function. It won't save lines. But will avoid duplicate logic that
needs to be kept in sync whenever there are future changes (fragile).

static inline void skb_set_delivery_type_by_clockid(struct sk_buff *skb,
						    ktime_t kt, clockid_t clockid)
{
	u8 tstamp_type = SKB_CLOCK_REAL;

	switch(clockid) {
	case CLOCK_REALTIME:
		break;
	case CLOCK_MONOTONIC:
		tstamp_type = SKB_CLOCK_MONO;
		break;
	default:
		WARN_ON_ONCE(1);
		kt = 0;
	};

	skb_set_delivery_type(skb, kt, tstamp_type);
}


> +	skb->tstamp = kt;
> +
> +	if (!kt) {
> +		skb->tstamp_type = SKB_CLOCK_REALTIME;
> +		return;
> +	}
> +
> +	switch (clockid) {
> +	case CLOCK_REALTIME:
> +		skb->tstamp_type = SKB_CLOCK_REALTIME;
> +		break;
> +	case CLOCK_MONOTONIC:
> +		skb->tstamp_type = SKB_CLOCK_MONOTONIC;
> +		break;
> +	}
> +}
> +
>  static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
> -					 bool mono)
> +					  u8 tstamp_type)

Indentation change: error?

> @@ -9444,7 +9444,7 @@ static struct bpf_insn *bpf_convert_tstamp_read(const struct bpf_prog *prog,
>  					TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
>  		*insn++ = BPF_JMP32_IMM(BPF_JNE, tmp_reg,
>  					TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK, 2);
> -		/* skb->tc_at_ingress && skb->mono_delivery_time,
> +		/* skb->tc_at_ingress && skb->tstamp_type:1,

Is the :1 a stale comment after we discussed how to handle the 2-bit
field going forward? I.e., not by ignoring the second bit.



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ