Re: CVS commit: src/sys/compat/linux/common
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(>f_lock); > uio.uio_offset = fp->f_offset; > mutex_exit(>f_lock); > ... > mutex_enter(>f_lock); > fp->f_offset = uio.uio_offset; > mutex_exit(>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)
Re: CVS commit: src/sys/compat/linux/common
On 2023-08-25 13:13, Taylor R Campbell wrote:
>> Module Name:src
>> Committed By: christos
>> Date: Wed Aug 23 19:17:59 UTC 2023
>>
>> Modified Files:
>> src/sys/compat/linux/common: linux_inotify.c
>>
>> Log Message:
>> put variable length structure at the end, so that clang does not complain.
>>
>> struct inotify_entry {
>> TAILQ_ENTRY(inotify_entry) ie_entries;
>> + charie_name[NAME_MAX + 1];
>> struct linux_inotify_event ie_event;
>> - charie_name[NAME_MAX+1];
>> };
>
> This can't be right, and it's a little unsettling that the problem
> isn't caught by any automatic tests.
>
> As I understand it, the `ie_name' member is supposed to provide
> storage for the `ie_event.name' array.
>
> So this looks like this change is going to lead either to a buffer
> overrun -- reading someone else's heap memory, or scribbling all over
> someone else's heap memory -- or to uninitialized or zero-filled
> memory being published to userland where there should be a pathname.
I don't think so. Notice that in inotify_read() two calls to uiomove()
are made, one on ie_event and one on ie_name... and in general this code
never accesses ie_event.name directly.
The reason I separated the fields in the first place was to make
allocation/deallocation simpler, not to use ie_name as storage for
ie_event.name.
Theo(dore)
Re: CVS commit: src
On 2023-07-30 10:51, Christos Zoulas wrote: > +Theodore, we'll add it! > > christos > >> On Jul 30, 2023, at 10:46 AM, Tobias Nygren wrote: >> >> On Fri, 28 Jul 2023 14:19:02 -0400 >> "Christos Zoulas" wrote: >> >>> Log Message: >>> Add epoll(2) from Theodore Preduta as part of GSoC 2023 >> >> The manual page mentions EPOLL_CLOEXEC is supported, but this >> flag is not defined in any header nor used in any code. >> So is it really supported? >> >> Absence of this flag makes devel/libevent from pkgsrc fail to build -- >> but should it really use epoll in the first place or should >> we change it to prefer kqueue when both are available? > Whoops! Fixed. Link: https://github.com/6167656e74323431/gsoc-netbsd-linux-emulation/commit/91c78e061cc2d9d78329377202d49fc64f78b94d Theo(dore)
