Implement kcmp(2)

[kib]

Summary

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

[kib]

Pass over the missed fileops.

[kib]

Microoptimize kcmp_getfp() for curproc case.

[yuripv]

include/unistd.h

523 ↗

(On Diff #133077)

looks like there's stray (); at the end of the prototype

[kib]

Remove stray ();

[manu]

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

[kib]

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.

[manu]

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.

[manu]

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

[pstef]

FWIW, Chromium also complains about the lack of kcmp.

sys/kern/sys_generic.c

2089 ↗

(On Diff #133087)

style(9) wants a blank line here

[kib]

Simplify error recovery logic.
Fix style issue.

[markj]

This is missing a manual page.

sys/compat/linuxkpi/common/src/linux_compat.c

1729 ↗

(On Diff #133137)

Shouldn't we compare f_ops as well? DTYPE_DEV is "device-specific fd type", not "has a linux cdev".

1734 ↗

(On Diff #133137)

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 ↗

(On Diff #133137)

Why CANDEBUG and not just CANSEE? I'd expect the check to be the same as what sysctl_kern_file() uses.

2124 ↗

(On Diff #133137)

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 ↗

(On Diff #133137)

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

498

Why EBADF instead of ENODEV?

[brooks]

sys/kern/sys_generic.c

2130 ↗

(On Diff #133137)

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.

[brooks]

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

[kib]

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 ↗

(On Diff #133137)

Nothing else sets type to DTYPE_DEV. I think we should rename the file type to e.g. DTYPE_LINUXKPI_DEV.

1734 ↗

(On Diff #133137)

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 ↗

(On Diff #133137)

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

498

fo_cmp() should be NULL only for badfileops, but I do not see an importance there.

[kib]

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

[markj]

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 ↗

(On Diff #133137)

That's fine with me.

1734 ↗

(On Diff #133137)

I think I misread. It appears possible for f_cdev to be NULL, but that's ok.

sys/sys/file.h

498

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.