linux-kernel - Re: [PATCH bpf-next v8 1/3] net: Rename mono_delivery_time to tstamp

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] [day] [month] [year] [list]

Message-ID: <66563b459e5de_2a7f7e2949d@willemb.c.googlers.com.notmuch>
Date: Tue, 28 May 2024 16:15:01 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
Cc: kernel@...cinc.com, 
 Willem de Bruijn <willemb@...gle.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>, 
 Martin KaFai Lau <martin.lau@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v8 1/3] net: Rename mono_delivery_time to
 tstamp_type for scalabilty

Martin KaFai Lau wrote:
> On 5/28/24 10:24 AM, Willem de Bruijn wrote:
> > Abhishek Chauhan (ABC) wrote:
> > 
> >>> +static inline void skb_set_delivery_type_by_clockid(struct sk_buff *skb,
> >>> +						    ktime_t kt, clockid_t clockid)
> >>> +{
> >>> +	u8 tstamp_type = SKB_CLOCK_REALTIME;
> >>> +
> >>> +	switch (clockid) {
> >>> +	case CLOCK_REALTIME:
> >>> +		break;
> >>> +	case CLOCK_MONOTONIC:
> >>> +		tstamp_type = SKB_CLOCK_MONOTONIC;
> >>> +		break;
> >>> +	default:
> >>
> >> Willem and Martin, I was thinking we should remove this warn_on_once from below line. Some systems also use panic on warn.
> >> So i think this might result in unnecessary crashes.
> >>
> >> Let me know what you think.
> >>
> >> Logs which are complaining.
> >> https://syzkaller.appspot.com/x/log.txt?x=118c3ae8980000
> > 
> > I received reports too. Agreed that we need to fix these reports.
> > 
> > The alternative is to limit sk_clockid to supported ones, by failing
> > setsockopt SO_TXTIME on an unsupported clock.
> > 
> > That changes established ABI behavior. But I don't see how another
> > clock can be used in any realistic way anyway.
> > 
> > Putting it out there as an option. It's riskier, but in the end I
> > believe a better fix than just allowing this state to continue.
> 
> Failing early would be my preference also. The current ABI is arguably at least 
> confusing (if not broken) considering other clockid is silently ignored by the 
> kernel.
>
> > 
> > A third option would be to not fail the system call, but silently
> > fall back to CLOCK_REALTIME. Essentially what happens in the datapath
> > in skb_set_delivery_type_by_clockid now. That is surprising behavior,
> > we should not do that.
> 
> Not sure if it makes sense to go back to this option only after there is 
> breakage report with a legit usage?

Agreed. We cannot break users. But I don't see how there are real
users for the current permissive API.