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

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Module Name:    src
> Committed By:   christos
> Date:           Thu Aug 24 19:51:24 UTC 2023
> 
> Modified Files:
>         src/sys/compat/linux/common: linux_inotify.c
> 
> Log Message:
> fix a locking bug (Theodore Preduta)
> 
>         if (needs_lock)
>                 vn_lock(vp, LK_SHARED | LK_RETRY);
> +       else
> +               /*
> +                * XXX We need to temprarily drop v_interlock because
> +                * it may be temporarily acquired by biowait().
> +                */
> +               mutex_exit(vp->v_interlock);
> +       KASSERT(!mutex_owned(vp->v_interlock));
>         error = VOP_READDIR(vp, &uio, fp->f_cred, &eofflag, NULL, NULL);
>         if (needs_lock)
>                 VOP_UNLOCK(vp);
> +       else
> +               mutex_enter(vp->v_interlock);

This looks questionable.

1. Why is v_interlock held in the first place?

2. Why is it safe to release v_interlock here, if the caller holds it?

3. What is the lock order for all the locks involved?

I see that inotify_readdir via get_inotify_dir_entries has two call
sites, leading to the following lock orders:

- linux_sys_inotify_add_watch
  -> mutex_enter(&ifd->ifd_lock)
  -> get_inotify_dir_entries(..., needs_lock=true)
     -> inotify_readdir(..., needs_lock=true)
        -> vn_lock(vp)
        -> VOP_READDIR(vp)

- handle_write
  -> mutex_enter(&ifd->ifd_lock)
  -> get_inotify_dir_entries(..., needs_lock=false)
     -> inotify_readdir(..., needs_lock=true)
        -> mutex_exit(vp->v_interlock)
        -> VOP_READDIR(vp)

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).

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.