Details

Reviewers

brooks
markj

Commits

rGf006524d6d69: kcmp(2): implement for procdesc
rG5c41d888de1a: kcmp(2): implement for devfs files
rG41fb6dc3d4df: kcmp(2): implement for linuxkpi cdevs
rGf04220c1b064: kcmp(2): implement for vnode files
rGf28526e9466c: kcmp(2): implement for generic file types
rG211bdd601ee5: Add kcmp(2) userspace bits
rGd8decc9ae31a: Add kcmp(2) kernel bits
rG168c7580c632: file: add fo_cmp method
rG58d317169834: Add fget_remote()
rG24fee9771e0b: sys/file.h: style

Summary

I also added KCMP_FILEOBJ which compares the objects files referring to, instead of just files itself as required by KCMP_FILE semantic.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Jan 19 2024, 10:23 PM

Herald added a reviewer: brooks. · View Herald TranscriptJan 19 2024, 10:23 PM

Herald added subscribers: linuxkpi, emaste, imp. · View Herald Transcript

kib requested review of this revision.Jan 19 2024, 10:23 PM

Pass over the missed fileops.

Microoptimize kcmp_getfp() for curproc case.

yuripv added a subscriber: yuripv.Jan 20 2024, 4:41 AM

yuripv added inline comments.

include/unistd.h
523	looks like there's stray `();` at the end of the prototype

Remove stray ();

arrowd added a subscriber: arrowd.Jan 20 2024, 7:19 AM

I have a few compile errors :

/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: error: declaration of 'struct kcmp_args' will not be visible outside of this function [-Werror,-Wvisibility]
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:27: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                               ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:38: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                                          ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:49: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                                                     ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2196:9: error: incomplete definition of type 'struct kcmp_args'
 2196 |             uap->idx1, uap->idx2));
      |             ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2196:20: error: incomplete definition of type 'struct kcmp_args'
 2196 |             uap->idx1, uap->idx2));
      |                        ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:1: error: no previous prototype for function 'sys_kcmp' [-Werror,-Wmissing-prototypes]
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      | ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2192:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
 2192 | int
      | ^
      | static 
7 errors generated.
--- sys_generic.o ---
*** [sys_generic.o] Error code 1

In D43518#992320, @manu wrote:

I have a few compile errors :

/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: error: declaration of 'struct kcmp_args' will not be visible outside of this function [-Werror,-Wvisibility]
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:27: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                               ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:38: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                                          ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:49: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                                                     ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2196:9: error: incomplete definition of type 'struct kcmp_args'
 2196 |             uap->idx1, uap->idx2));
      |             ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2196:20: error: incomplete definition of type 'struct kcmp_args'
 2196 |             uap->idx1, uap->idx2));
      |                        ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:1: error: no previous prototype for function 'sys_kcmp' [-Werror,-Wmissing-prototypes]
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      | ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2192:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
 2192 | int
      | ^
      | static 
7 errors generated.
--- sys_generic.o ---
*** [sys_generic.o] Error code 1

You need to do make sysent at src/ top level before starting buildworld or buildkernel.

In D43518#992377, @kib wrote:

In D43518#992320, @manu wrote:

I have a few compile errors :

/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: error: declaration of 'struct kcmp_args' will not be visible outside of this function [-Werror,-Wvisibility]
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:27: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                               ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:38: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                                          ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2195:49: error: incomplete definition of type 'struct kcmp_args'
 2195 |         return (kern_kcmp(td, uap->pid1, uap->pid2, uap->type,
      |                                                     ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2196:9: error: incomplete definition of type 'struct kcmp_args'
 2196 |             uap->idx1, uap->idx2));
      |             ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2196:20: error: incomplete definition of type 'struct kcmp_args'
 2196 |             uap->idx1, uap->idx2));
      |                        ~~~^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:36: note: forward declaration of 'struct kcmp_args'
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      |                                    ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2193:1: error: no previous prototype for function 'sys_kcmp' [-Werror,-Wmissing-prototypes]
 2193 | sys_kcmp(struct thread *td, struct kcmp_args *uap)
      | ^
/usr/home/manu/Work/freebsd/src/upstream_main/sys/kern/sys_generic.c:2192:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
 2192 | int
      | ^
      | static 
7 errors generated.
--- sys_generic.o ---
*** [sys_generic.o] Error code 1

You need to do make sysent at src/ top level before starting buildworld or buildkernel.

Ah yes sorry, always forget that for new syscalls.
Will try again today and will cook a patch for mesa to test it.

Seems to work fine with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27184 applied.
This part of FreeBSD is a bit to hard for me to review though :)

FWIW, Chromium also complains about the lack of kcmp.

sys/kern/sys_generic.c
2089	style(9) wants a blank line here

kib marked an inline comment as done.Jan 22 2024, 4:58 AM

Simplify error recovery logic.
Fix style issue.

kib added a reviewer: markj.Jan 22 2024, 5:59 AM

zlei added a subscriber: zlei.Jan 22 2024, 8:11 AM

This is missing a manual page.

sys/compat/linuxkpi/common/src/linux_compat.c
1729	Shouldn't we compare f_ops as well? DTYPE_DEV is "device-specific fd type", not "has a linux cdev".
1734	fd_install() in the LinuxKPI installs a DTYPE_DEV file without setting f_cdev. I'm not sure if the comparison is right.
sys/kern/sys_generic.c
2093	Why CANDEBUG and not just CANSEE? I'd expect the check to be the same as what sysctl_kern_file() uses.
2124	Rather than exporting fdhold() and fddrop(), I'd make this a function in kern_descrip.c, maybe fget_proc(). There is nothing specific to kcmp() here.
2130	Why is it "unsigned long" instead of "int"? I understand that Linux defines it that way, but it's weird. Ah, for some uses, one of the argument is a pointer. Can we use uintptr_t instead then? I believe it's compatible for the cases we care about, and it'll let you remove some casts below.
sys/sys/file.h
500	Why EBADF instead of ENODEV?

brooks added inline comments.Jan 22 2024, 7:45 PM

sys/kern/sys_generic.c
2130	Agree on uintptr_t for idx1 and idx2. It's a shame that Linux defines them to be ignored rather than 0 when unused, but so it goes.

FWIW, this Debian code search query suggests that we could enforce not passing nonsense values to idx[12] when they are ignored. That's probably not a good idea though. https://codesearch.debian.net/search?q=SYS_kcmp+-path%3A.*%2Fmusl%2F.*+-path%3A.*%2Flibc%2F.*+-path%3A.*%2Flinux_like%2F.*+-file%3Apo+-file%3Apot&literal=0

In D43518#992807, @markj wrote:

This is missing a manual page.

Indeed. Problem is that I did read the man page to write the implementation, which raises the question of 'clean room' text content. It is similar to membarrier(2).

sys/compat/linuxkpi/common/src/linux_compat.c
1729	Nothing else sets type to DTYPE_DEV. I think we should rename the file type to e.g. DTYPE_LINUXKPI_DEV.
1734	I do not follow. finit() in Linuxkpi always set f_data to filp, which is populated with f_cdev pointing to lcdev.
sys/kern/sys_generic.c
2093	Linux kcmp(2) manpage states: Permission to employ kcmp() is governed by ptrace access mode PTRACE_MODE_READ_REALCREDS checks against both pid1 and pid2; see ptrace(2).
sys/sys/file.h
500	fo_cmp() should be NULL only for badfileops, but I do not see an importance there.

fget_remote()
unsigned long->uintptr_t idx
More minor tweaks like error codes returned

In D43518#993020, @kib wrote:

In D43518#992807, @markj wrote:

This is missing a manual page.

Indeed. Problem is that I did read the man page to write the implementation, which raises the question of 'clean room' text content. It is similar to membarrier(2).

I didn't really look at the Linux man page, so took the liberty of writing one: https://reviews.freebsd.org/D43562

sys/compat/linuxkpi/common/src/linux_compat.c
1729	That's fine with me.
1734	I think I misread. It appears possible for f_cdev to be NULL, but that's ok.
sys/sys/file.h
500	Well, it distinguishes the "fd is not valid" case from "fd is not a supported type" case. If we think the latter case is impossible, then there should be an assertion instead.