Date: Thu, 24 Aug 2023 16:31:28 -0600 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Warner Losh <imp@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: af93fea71038 - main - timerfd: Move implementation from linux compat to sys/kern Message-ID: <CANCZdfo0_K6SedMfEAAkWFMFXMd146WyWKUJzzytGVfKA_8ZDg@mail.gmail.com> In-Reply-To: <ZOfXKlzSG0jYDU53@kib.kiev.ua> References: <202308242029.37OKTmVs091755@gitrepo.freebsd.org> <ZOfXKlzSG0jYDU53@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <kostikbel@gmai= l.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=3Daf93fea710385b2b11f0cabd377e7ed= 6f3d97c34 > > > > commit af93fea710385b2b11f0cabd377e7ed6f3d97c34 > > Author: Jake Freeland <jfree@freebsd.org> > > AuthorDate: 2023-08-24 04:39:54 +0000 > > Commit: Warner Losh <imp@FreeBSD.org> > > 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 <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Thu, Aug 24, 2023 at 4:18=E2=80=AF= PM Konstantin Belousov <<a href=3D"mailto:kostikbel@gmail.com">kostikbel= @gmail.com</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style= =3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding= -left:1ex">On Thu, Aug 24, 2023 at 08:29:48PM +0000, Warner Losh wrote:<br> > The branch main has been updated by imp:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Daf93fea71038= 5b2b11f0cabd377e7ed6f3d97c34" rel=3D"noreferrer" target=3D"_blank">https://= cgit.FreeBSD.org/src/commit/?id=3Daf93fea710385b2b11f0cabd377e7ed6f3d97c34<= /a><br> > <br> > commit af93fea710385b2b11f0cabd377e7ed6f3d97c34<br> > Author:=C2=A0 =C2=A0 =C2=A0Jake Freeland <<a href=3D"mailto:jfree@f= reebsd.org" target=3D"_blank">jfree@freebsd.org</a>><br> > AuthorDate: 2023-08-24 04:39:54 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2023-08-24 20:28:56 +0000<br> > <br> >=C2=A0 =C2=A0 =C2=A0timerfd: Move implementation from linux compat to s= ys/kern<br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0Move the timerfd impelemntation from linux compat c= ode to sys/kern. Use<br> >=C2=A0 =C2=A0 =C2=A0it to implement the new system calls for timerfd. A= dd a hook to kern_tc<br> >=C2=A0 =C2=A0 =C2=A0to allow timerfd to know when the system time has s= tepped. Add kqueue<br> >=C2=A0 =C2=A0 =C2=A0support to timerfd. Adjust a few names to be less L= inux centric.<br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0RelNotes: YES<br> >=C2=A0 =C2=A0 =C2=A0Reviewed by: markj (on irc), imp, kib (with reserva= tions), jhb (slack)<br> >=C2=A0 =C2=A0 =C2=A0Differential Revision: <a href=3D"https://reviews.f= reebsd.org/D38459" rel=3D"noreferrer" target=3D"_blank">https://reviews.fre= ebsd.org/D38459</a><br> >[snip]</blockquote><div><br></div><div>In my haste to get this in, I ne= glected to clear the commit message with Jake, and missed a few things that= </div><div>he refactored. He pointed out after I pushed the change:</div><d= iv><div><br class=3D"gmail-Apple-interchange-newline"></div></div></div><bl= ockquote style=3D"margin:0 0 0 40px;border:none;padding:0px"><div class=3D"= gmail_quote"><div><div>One minor complaint, though. Your commit message fai= ls to recognize</div></div></div><div class=3D"gmail_quote"><div><div>how m= uch refactoring work I put into timerfd. I refactored a lot of important</d= iv></div></div><div class=3D"gmail_quote"><div><div>bits in the native vers= ion. The previous Linux-compat version was inaccurate</div></div></div><div= class=3D"gmail_quote"><div><div>and had functional inconsistencies with Li= nux's timerfd.</div></div></div><div class=3D"gmail_quote"><div><div><b= r></div></div></div><div class=3D"gmail_quote"><div><div>Just to list them,= I added:</div></div></div><div class=3D"gmail_quote"><div><div>* Time jump= handling through the TFD_TIMER_CANCEL_ON_SET flag.</div></div></div><div c= lass=3D"gmail_quote"><div><div>* Accuracy improvements (the Linux-compat im= plementation would deviate by seconds).</div></div></div><div class=3D"gmai= l_quote"><div><div>* Guards for DoS attacks so timerfd could account for mi= ssed events.</div></div></div><div class=3D"gmail_quote"><div><div>* Consis= tent error handling and expected return values that parody Linux.</div></di= v></div><div class=3D"gmail_quote"><div><div>* Descriptor stat() support.</= div></div></div></blockquote><div class=3D"gmail_quote"><div><div><br></div= ><div>for which I offer my profuse apologies for the oversight.</div></div>= <div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px = 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I did a = very quick look over the added code.<br> <br> I do not see any protection for the timerfd_head list manipulation.<br> <br> It is not clear what is protected by tfd->tfd_lock: e.g. in timerfd_stat= ()<br> it covers reading of items, writing of which is not protected by the mtx,<b= r> everything except tfd_atim.<br> There is no annotations in the timer structure for the locking regime.<br><= /blockquote><div><br></div><div>That's a good point. I'll work with= Jake to add them.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" = style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);pa= dding-left:1ex"> stat st_ctim is always zero, this is somewhat strange.<br></blockquote><div= ><br></div><div>I'm not sure what's going on there.</div><div>=C2= =A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8e= x;border-left:1px solid rgb(204,204,204);padding-left:1ex"> The<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 tfd =3D fp->f_data;<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (tfd =3D=3D NULL || fp->f_type !=3D DTYPE= _TIMERFD) {<br> triggers UB when f_type is not DTYPE_TIMERFD.<br></blockquote><div><br></di= v><div>Good catch. I didn't notice.</div><div>=C2=A0</div><blockquote c= lass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px soli= d rgb(204,204,204);padding-left:1ex"> compat32 stuff was put into the sys/kern instead of sys/compat/freebsd32.<b= r> sys/timerfd.h pollutes userspace with sys/proc.h.<br></blockquote><div><br>= </div><div>I don't think that was intentional. I didn't notice, but= you are right on both of these.</div><div>I'll work with Jake to fix. = The second one is easier to fix.</div><div>=C2=A0</div><blockquote class=3D= "gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(2= 04,204,204);padding-left:1ex"> The regenerated files were put in the same commit as (probably) human<br> written files, why?<br></blockquote><div><br></div><div>That's my mista= ke. With brooks' new checks in Cirrus CI, I thought=C2=A0 we'd tran= sitioned to</div><div>doing that in the same commit. I misunderstood.</div>= <div><br></div><div>Warner</div></div></div> --000000000000aac0f30603b2ca1b--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfo0_K6SedMfEAAkWFMFXMd146WyWKUJzzytGVfKA_8ZDg>