Page MenuHomeFreeBSD

Implement kcmp(2)
ClosedPublic

Authored by kib on Jan 19 2024, 10:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jul 21, 3:51 PM
Unknown Object (File)
Tue, Jul 16, 2:05 PM
Unknown Object (File)
Wed, Jul 3, 8:42 AM
Unknown Object (File)
Sun, Jun 30, 10:10 PM
Unknown Object (File)
Sun, Jun 30, 10:10 PM
Unknown Object (File)
Sun, Jun 30, 10:10 PM
Unknown Object (File)
Sun, Jun 30, 10:10 PM
Unknown Object (File)
Sun, Jun 30, 10:10 PM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 inline comments.
include/unistd.h
523 ↗(On Diff #133077)

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

kib marked an inline comment as done.

Remove stray ();

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 ↗(On Diff #133087)

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.

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?

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.

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 marked 6 inline comments as done.Jan 22 2024, 10:37 PM

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 marked 3 inline comments as done.

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

In D43518#993020, @kib 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.

This revision is now accepted and ready to land.Jan 23 2024, 3:37 PM