Page MenuHomeFreeBSD
Differential D43518

Implement kcmp(2)
ClosedPublic
Actions

Authored by kib on Jan 19 2024, 10:23 PM.
Tags
None
Referenced Files
F103229532: D43518.diff
Fri, Nov 22, 10:57 AM
Unknown Object (File)
Tue, Nov 19, 7:00 PM
Unknown Object (File)
Fri, Nov 8, 8:42 PM
Unknown Object (File)
Fri, Nov 8, 8:00 PM
Unknown Object (File)
Fri, Nov 8, 7:41 PM
Unknown Object (File)
Fri, Nov 8, 7:27 PM
Unknown Object (File)
Fri, Nov 8, 7:27 PM
Unknown Object (File)
Fri, Nov 8, 7:27 PM
View All Files
Subscribers
arrowd
emaste
imp
manu
pstef
yuripv
zlei
linuxkpi

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
kib updated this revision to Diff 133072.Jan 19 2024, 10:46 PM

Pass over the missed fileops.

kib updated this revision to Diff 133077.Jan 20 2024, 12:52 AM

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

kib updated this revision to Diff 133087.Jan 20 2024, 4:53 AM
kib marked an inline comment as done.

Remove stray ();

arrowd added a subscriber: arrowd.Jan 20 2024, 7:19 AM
manu added a subscriber: manu.Jan 20 2024, 7:56 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
kib added a comment.Jan 20 2024, 6:01 PM
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 added a comment.Jan 21 2024, 8:04 AM
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 added a comment.Jan 21 2024, 2:06 PM

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 added a subscriber: pstef.Jan 21 2024, 7:17 PM

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
kib updated this revision to Diff 133137.Jan 22 2024, 5:40 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
markj added a comment.Jan 22 2024, 2:42 PM

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
502

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.

brooks added a comment.Jan 22 2024, 7:54 PM

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
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
502

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

kib updated this revision to Diff 133175.Jan 22 2024, 10:45 PM
kib marked 3 inline comments as done.

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

markj added a comment.Jan 23 2024, 3:35 PM
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
502

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.

markj accepted this revision.Jan 23 2024, 3:37 PM
This revision is now accepted and ready to land.Jan 23 2024, 3:37 PM
brooks accepted this revision.Jan 23 2024, 5:46 PM
Closed by commit rG24fee9771e0b: sys/file.h: style (authored by kib). · Explain WhyJan 24 2024, 5:12 AM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rG24fee9771e0b: sys/file.h: style.
kib added a commit: rG58d317169834: Add fget_remote().
kib added a commit: rG168c7580c632: file: add fo_cmp method.
kib added a commit: rGd8decc9ae31a: Add kcmp(2) kernel bits.
kib added a commit: rG211bdd601ee5: Add kcmp(2) userspace bits.
kib added a commit: rGf28526e9466c: kcmp(2): implement for generic file types.
kib added a commit: rGf04220c1b064: kcmp(2): implement for vnode files.
kib added a commit: rG5c41d888de1a: kcmp(2): implement for devfs files.
kib added a commit: rG41fb6dc3d4df: kcmp(2): implement for linuxkpi cdevs.
kib added a commit: rGf006524d6d69: kcmp(2): implement for procdesc.

Revision Contents
Changeset List

PathSize
include/
1 line
lib/
libc/
sys/
4 lines
sys/
compat/
linuxkpi/
common/
src/
14 lines
fs/
devfs/
9 lines
kern/
33 lines
1 line
1 line
95 lines
1 line
14 lines
1 line
1 line
10 lines
1 line
1 line
1 line
1 line
9 lines
sys/
17 lines
2 lines
2 lines
7 lines
1 line

Diff 133175

View Options

include/unistd.h

Loading...
View Options

lib/libc/sys/Symbol.map

Loading...
View Options

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

Loading...
View Options

sys/fs/devfs/devfs_vnops.c

Loading...
View Options

sys/kern/kern_descrip.c

Loading...
View Options

sys/kern/kern_event.c

Loading...
View Options

sys/kern/sys_eventfd.c

Loading...
View Options

sys/kern/sys_generic.c

Loading...
View Options

sys/kern/sys_pipe.c

Loading...
View Options

sys/kern/sys_procdesc.c

Loading...
View Options

sys/kern/sys_socket.c

Loading...
View Options

sys/kern/sys_timerfd.c

Loading...
View Options

sys/kern/syscalls.master

Loading...
View Options

sys/kern/tty_pts.c

Loading...
View Options

sys/kern/uipc_mqueue.c

Loading...
View Options

sys/kern/uipc_sem.c

Loading...
View Options

sys/kern/uipc_shm.c

Loading...
View Options

sys/kern/vfs_vnops.c

Loading...
View Options

sys/sys/file.h

Loading...
View Options

sys/sys/syscallsubr.h

Loading...
View Options

sys/sys/systm.h

Loading...
View Options

sys/sys/unistd.h

Loading...
View Options

sys/sys/vnode.h

Loading...