Re: CVS commit: src/sys/compat/linux/common

[Theodore Preduta <theo%pta.gg@localhost>]

On 2023-08-25 13:30, Taylor R Campbell wrote:
> Since VOP_READDIR requires vp to be locked, I can infer that the
> handle_write caller must already hold vp locked.  But that means that
> we have ifd_lock -> vnode lock in one path, and vnode lock -> ifd_lock
> in another path, which is forbidden (unless there's some reason we can
> prove these paths never happen concurrently).

Hmm... I did not realize this, so I don't think NOTE_WRITE
disambiguation is (or has ever been) safe.

The real pain here is that we inherit a held vp_interlock and vnode lock
in the needs_lock=false case from the kevent callback.  So this may
require a pretty substantial locking revision(?)

Anyways, I'll take a closer look later this weekend.

> I'm also suspicious of logic like this:
> 
>         mutex_enter(&fp->f_lock);
>         uio.uio_offset = fp->f_offset;
>         mutex_exit(&fp->f_lock);
> ...
>         mutex_enter(&fp->f_lock);
>         fp->f_offset = uio.uio_offset;
>         mutex_exit(&fp->f_lock);
> 
> If fp->f_offset can change concurrently while f_lock is released, this
> looks like a problem.  But if it can't, why do we need the lock at
> all?  I suspect this really does need a lock over the whole
> transaction (maybe fp->f_lock, maybe something else), which is not
> released while I/O is happening -- possibly not with mutex(9) but
> something interruptible instead.

I think you're right about needing a lock for the whole transaction.
Since this is called from get_inotify_dir_entries(), perhaps it would be
better for get_inotify_dir_entries() to keep track of the offset and
pass it in instead?

Theo(dore)