Re: git: af93fea71038 - main - timerfd: Move implementation from linux compat to sys/kern

On Thu, Aug 24, 2023 at 4:18 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=af93fea710385b2b11f0cabd377e7ed6f3d97c34
> >
> > 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 kqueue
> >     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 inaccurate
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 = fp->f_data;
>         if (tfd == NULL || fp->f_type != 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