From nobody Thu Aug 24 22:31:28 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4RWyVj3pTyz4rfpP for ; Thu, 24 Aug 2023 22:31:41 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ed1-x533.google.com (mail-ed1-x533.google.com [IPv6:2a00:1450:4864:20::533]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RWyVj16cLz3cFt for ; Thu, 24 Aug 2023 22:31:41 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-x533.google.com with SMTP id 4fb4d7f45d1cf-52a4b62c2f5so483873a12.1 for ; Thu, 24 Aug 2023 15:31:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20221208.gappssmtp.com; s=20221208; t=1692916299; x=1693521099; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=/p27l67Kn1HIoXp/f59LPHhlkBLW4+1enNUjjEwJAaM=; b=l+OFeHrxiGjDfVbkjEUK1WxjC1qY2hEdT7jmUKCqJxkNW9Dq8c3M3NaT6pK3EQGLEZ PiL6YmqgEtvFfp54RBvQwf7uj1MjgdGGcc2BpVkaX+LOdLPyK7KFGa+XDfQdeRUlM/Yc sPQgrNLRRRyg+TegzZB+SNzn4aWUcD4H/xdY5M/URnpMZebeC90843HHa9uqoogh35yr iCf45/vWCbqUY4T1VwYI8qsJp7hYtRiIwflxljIIaWOm27cNcLLU3RBuvNZ5fVnde4Jq vLTzpC2EhBzmpSC61T8WMp4oZE0URD+iPLqnIK9sTbMkYSBvQq9UYgGcvNaEesOG1bgV ZZLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692916299; x=1693521099; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=/p27l67Kn1HIoXp/f59LPHhlkBLW4+1enNUjjEwJAaM=; b=Iyq6lpRTd7rHpyFRwpyI6liALutPNW/E8gmiYtB/zwet0vsteCdYjJtCXyjn5WVogS /qF1ZUrJHYMDv8miNIlp2AMWWdlGwdrjyQeh8WQgNrxfSWr689yWPCBNbZ0/6BS2HjeQ vZiegkucQsW4Rxj3tayDY82N2JfqQILhHzLbcaad/bzAsKuzs+nhxk+G49EbEsZmREhw ivdwxSqhI3WHj0HxZ14L0PWrkldfjFcIZQym9MsOTbv6Gk72a3utPNQxvB143CleE8Kd xCAI4zzIkWcomxbUfxjNvQWB5/iaLpBboz7l59DpxSEvBBrqtBUdVajoI4aWMIoAQzQX 90yw== X-Gm-Message-State: AOJu0Yx6rejknhe7WwNKtdkWz0nyqicmu3VV6GyWLWNXYMsSKUfHDr+U aLQOEWNHAG2YiXL5i4sSTrERVbdwvBGQzaDwTcDvvA== X-Google-Smtp-Source: AGHT+IFIvVy3Lr4XQrflXRfQXFNpH4qSj85FdCz4N9joF7DmN/XRZLC9VGoCp1T8hkYxkfP2oJCNb7jXoH9Rv4Hl1As= X-Received: by 2002:aa7:dad5:0:b0:523:2847:fb5a with SMTP id x21-20020aa7dad5000000b005232847fb5amr11467646eds.40.1692916299505; Thu, 24 Aug 2023 15:31:39 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202308242029.37OKTmVs091755@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Thu, 24 Aug 2023 16:31:28 -0600 Message-ID: Subject: Re: git: af93fea71038 - main - timerfd: Move implementation from linux compat to sys/kern To: Konstantin Belousov Cc: Warner Losh , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="000000000000aac0f30603b2ca1b" X-Rspamd-Queue-Id: 4RWyVj16cLz3cFt X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] --000000000000aac0f30603b2ca1b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Aug 24, 2023 at 4:18=E2=80=AFPM Konstantin Belousov wrote: > On Thu, Aug 24, 2023 at 08:29:48PM +0000, Warner Losh wrote: > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3Daf93fea710385b2b11f0cabd377e7ed= 6f3d97c34 > > > > commit af93fea710385b2b11f0cabd377e7ed6f3d97c34 > > Author: Jake Freeland > > AuthorDate: 2023-08-24 04:39:54 +0000 > > Commit: Warner Losh > > CommitDate: 2023-08-24 20:28:56 +0000 > > > > timerfd: Move implementation from linux compat to sys/kern > > > > Move the timerfd impelemntation from linux compat code to sys/kern. > Use > > it to implement the new system calls for timerfd. Add a hook to > kern_tc > > to allow timerfd to know when the system time has stepped. Add kque= ue > > support to timerfd. Adjust a few names to be less Linux centric. > > > > RelNotes: YES > > Reviewed by: markj (on irc), imp, kib (with reservations), jhb > (slack) > > Differential Revision: https://reviews.freebsd.org/D38459 > >[snip] In my haste to get this in, I neglected to clear the commit message with Jake, and missed a few things that he refactored. He pointed out after I pushed the change: One minor complaint, though. Your commit message fails to recognize how much refactoring work I put into timerfd. I refactored a lot of important bits in the native version. The previous Linux-compat version was inaccurat= e and had functional inconsistencies with Linux's timerfd. Just to list them, I added: * Time jump handling through the TFD_TIMER_CANCEL_ON_SET flag. * Accuracy improvements (the Linux-compat implementation would deviate by seconds). * Guards for DoS attacks so timerfd could account for missed events. * Consistent error handling and expected return values that parody Linux. * Descriptor stat() support. for which I offer my profuse apologies for the oversight. > I did a very quick look over the added code. > > I do not see any protection for the timerfd_head list manipulation. > > It is not clear what is protected by tfd->tfd_lock: e.g. in timerfd_stat(= ) > it covers reading of items, writing of which is not protected by the mtx, > everything except tfd_atim. > There is no annotations in the timer structure for the locking regime. > That's a good point. I'll work with Jake to add them. > stat st_ctim is always zero, this is somewhat strange. > I'm not sure what's going on there. > The > tfd =3D fp->f_data; > if (tfd =3D=3D NULL || fp->f_type !=3D DTYPE_TIMERFD) { > triggers UB when f_type is not DTYPE_TIMERFD. > Good catch. I didn't notice. > compat32 stuff was put into the sys/kern instead of sys/compat/freebsd32. > sys/timerfd.h pollutes userspace with sys/proc.h. > I don't think that was intentional. I didn't notice, but you are right on both of these. I'll work with Jake to fix. The second one is easier to fix. > The regenerated files were put in the same commit as (probably) human > written files, why? > That's my mistake. With brooks' new checks in Cirrus CI, I thought we'd transitioned to doing that in the same commit. I misunderstood. Warner --000000000000aac0f30603b2ca1b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Aug 24, 2023 at 4:18=E2=80=AF= PM Konstantin Belousov <kostikbel= @gmail.com> wrote:
On Thu, Aug 24, 2023 at 08:29:48PM +0000, Warner Losh wrote:
> The branch main has been updated by imp:
>
> URL: https://= cgit.FreeBSD.org/src/commit/?id=3Daf93fea710385b2b11f0cabd377e7ed6f3d97c34<= /a>
>
> commit af93fea710385b2b11f0cabd377e7ed6f3d97c34
> Author:=C2=A0 =C2=A0 =C2=A0Jake Freeland <jfree@freebsd.org>
> AuthorDate: 2023-08-24 04:39:54 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org>
> CommitDate: 2023-08-24 20:28:56 +0000
>
>=C2=A0 =C2=A0 =C2=A0timerfd: Move implementation from linux compat to s= ys/kern
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0Move the timerfd impelemntation from linux compat c= ode to sys/kern. Use
>=C2=A0 =C2=A0 =C2=A0it to implement the new system calls for timerfd. A= dd a hook to kern_tc
>=C2=A0 =C2=A0 =C2=A0to allow timerfd to know when the system time has s= tepped. Add kqueue
>=C2=A0 =C2=A0 =C2=A0support to timerfd. Adjust a few names to be less L= inux centric.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0RelNotes: YES
>=C2=A0 =C2=A0 =C2=A0Reviewed by: markj (on irc), imp, kib (with reserva= tions), jhb (slack)
>=C2=A0 =C2=A0 =C2=A0Differential Revision: https://reviews.fre= ebsd.org/D38459
>[snip]

In my haste to get this in, I ne= glected to clear the commit message with Jake, and missed a few things that=
he refactored. He pointed out after I pushed the change:

One minor complaint, though. Your commit message fai= ls to recognize
how m= uch refactoring work I put into timerfd. I refactored a lot of important
bits in the native vers= ion. The previous Linux-compat version was inaccurate
and had functional inconsistencies with Li= nux's timerfd.
Just to list them,= I added:
* Time jump= handling through the TFD_TIMER_CANCEL_ON_SET flag.
* Accuracy improvements (the Linux-compat im= plementation would deviate by seconds).
* Guards for DoS attacks so timerfd could account for mi= ssed events.
* Consis= tent error handling and expected return values that parody Linux.
* Descriptor stat() support.

for which I offer my profuse apologies for the oversight.
=
=C2=A0
I did a = very quick look over the added code.

I do not see any protection for the timerfd_head list manipulation.

It is not clear what is protected by tfd->tfd_lock: e.g. in timerfd_stat= ()
it covers reading of items, writing of which is not protected by the mtx, everything except tfd_atim.
There is no annotations in the timer structure for the locking regime.
<= /blockquote>

That's a good point. I'll work with= Jake to add them.
=C2=A0
stat st_ctim is always zero, this is somewhat strange.

I'm not sure what's going on there.
=C2= =A0
The
=C2=A0 =C2=A0 =C2=A0 =C2=A0 tfd =3D fp->f_data;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (tfd =3D=3D NULL || fp->f_type !=3D DTYPE= _TIMERFD) {
triggers UB when f_type is not DTYPE_TIMERFD.

Good catch. I didn't notice.
=C2=A0
compat32 stuff was put into the sys/kern instead of sys/compat/freebsd32. sys/timerfd.h pollutes userspace with sys/proc.h.

=
I don't think that was intentional. I didn't notice, but= you are right on both of these.
I'll work with Jake to fix. = The second one is easier to fix.
=C2=A0
The regenerated files were put in the same commit as (probably) human
written files, why?

That's my mista= ke. With brooks' new checks in Cirrus CI, I thought=C2=A0 we'd tran= sitioned to
doing that in the same commit. I misunderstood.
=

Warner
--000000000000aac0f30603b2ca1b--