linux: Support POSIX message queues #1248

ricardobranco777 · 2024-05-17T18:53:02Z

This PR adds preliminary support for POSIX message queues. It also adds the respective /proc/sys/mqueue entries to linprocfs.

Not supported:

mq_notify with SIGEV_THREAD_ID as it must call a Linux function.

TODO:

Make new sysctl's read-write?
Support 32-bit on 64-bits?

Ref: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279069

sys/compat/linux/linux_timer.c

sys/compat/linux/linux_misc.c

ricardobranco777 · 2024-05-18T22:40:40Z

It's passing the LTP & POSIX tests on both 64-bits & 32-bits except for:

mq_open01.c:165: TBROK: Failed to close FILE '/proc/sys/fs/mqueue/queues_max': EBADF (9)
Mostly due to not being able to open this file read-write. Expected.
mq_notify03.c:57: TFAIL: mq_notify(m, &sev) failed: EINVAL (22) mq_notify03.c:64: TFAIL: mq_notify(m, &sev) failed: EINVAL (22)
Not supported calling Linux functions. Expected.
mq_timedsend01.c:206: TFAIL: mq_timedsend() failed unexpectedly, expected SUCCESS: EMSGSIZE (90) mq_timedsend01.c:206: TFAIL: mq_timedsend() failed unexpectedly, expected SUCCESS: EINVAL (22)
This may be a bug or feature in the FreeBSD implementation.

MISSING: Support for 32-bits on 64.

To test it I had to mount mqueuefs on a Ubuntu chroot created with sudo debootstrap jammy /compat/ubuntu.

sudo mkdir /compat/ubuntu/mnt/queue
sudo mount -t mqueuefs none /compat/ubuntu/mnt/queue

Note: Beware of removing any queue via the filesystem due to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278936 It's better to use posixmqcontrol for this.

On an Ubuntu 22.04 VM I compiled LTP like this:

sudo apt install build-essential automake autoconf pkg-config m4
git clone https://github.com/linux-test-project/ltp
cd ltp
make autotools
./configure --with-open-posix-testsuite --with-realtime-testsuite
make -j8
sudo make install

Then copied /opt/ltp to the chroot and ran:
for d in /opt/ltp/testcases/open_posix_testsuite/functional/mqueues testcases/open_posix_testsuite/conformance/interfaces/mq_* ; do cd $d ; ./run.sh ; cd - ; done

Also for i in /opt/ltp/testcases/bin/mq_* ; do $i ; done

sys/compat/linprocfs/linprocfs.c

kostikbel

I am not sure about wisdom of allowing '/' for FreeBSD native, although I do not object. Can you think of (negative) consequences of changing native behavior?

sys/kern/uipc_mqueue.c

kostikbel

Please remove generated diffs from the commit.

sys/i386/linux/syscalls.master

kostikbel

kern_ functions are normally put into sys/syscallsubr.h

kostikbel

Same, please put kern_ functions into sys/syscallsubr.h

sys/kern/uipc_mqueue.c

ricardobranco777 · 2024-05-19T08:38:54Z

Please remove generated diffs from the commit.

You mean the ones generated by make sysent? Fixed.

Had to run git stash push -m "make sysent" and remind me to apply it everytime I do a kernel build.

ricardobranco777 · 2024-05-19T08:41:13Z

I am not sure about wisdom of allowing '/' for FreeBSD native, although I do not object. Can you think of (negative) consequences of changing native behavior?

I don't see any. In fact, I was surprised mq_open allowed paths starting with //.

sys/compat/linux/linux_misc.c

sys/sys/syscallsubr.h

sys/compat/linux/linux_misc.c

kostikbel · 2024-05-21T04:14:44Z

sys/compat/linux/linux_misc.c

+	}
+
+	return (kern_kmq_open(td, args->name, flags, args->mode,
+	    args->attr != NULL ? &attr : NULL));


What if the O_CREAT flag is not set, but args->attr is not NULL? Wouldn't we pass a pointer to the stack garbage to kern_kmq_open?

What if the O_CREAT flag is not set, but args->attr is not NULL? Wouldn't we pass a pointer to the stack garbage to kern_kmq_open?

sys_kmq_open also does this. This function is based on sys_kmq_open. Fortunately, kern_kmq_open does the right thing.

I believe it is worth fixing. You can do it as a follow-up to the current PR.

ricardobranco777 · 2024-05-23T18:10:18Z

I'm landing this... There's a minor whitespace conflict that I can ignore....

Also fixed it. Hope I didn't interfere with any rebasing.

bsdimp · 2024-05-23T18:39:05Z

For Phabricator reviews, the advise to remove auto generated files is good.
For pull requests, it's terrible (at least for now). They should be included, otherwise automated testing is impossible without a crazy amount of DWIM. I'm going to respectfully disagree with that bit of advice in this context. If we wanted to go to signed pull requests, say, then that would make those impossible to land since it will break the build guaranteed (though honestly we should be generating most of the files make sysent does on the fly, not as checked in artifacts, but I digress).

brooksdavis · 2024-05-23T18:44:19Z

For Phabricator reviews, the advise to remove auto generated files is good.

I'll disagree with this statement. Phabricator hides the generated diff by default due to the @generated string so there is little value in not including them. It's unfortunate that ignoring files matching certain patterns isn't a git feature yet.

bsdimp · 2024-05-23T18:45:18Z

For Phabricator reviews, the advise to remove auto generated files is good.

I'll disagree with this statement. Phabricator hides the generated diff by default due to the @generated string so there is little value in not including them. It's unfortunate that ignoring files matching certain patterns isn't a git feature yet.

ah, I'll retract it for phab then. I'd forgotten it does that.

kostikbel · 2024-05-23T19:16:34Z

For Phabricator reviews, the advise to remove auto generated files is good.

I'll disagree with this statement. Phabricator hides the generated diff by default due to the @generated string so there is little value in not including them. It's unfortunate that ignoring files matching certain patterns isn't a git feature yet.

ah, I'll retract it for phab then. I'd forgotten it does that.

Autogenerated files make reading both reviews/PRs and actual commits awful *. They should be changed in the separate commit.

In principle, such commit can be included into the series forming the PR, but then it leaves the possibility of conflicting with the changes in upstream. I do not see any problem with committer doing make sysent and adding this last commit before pushing.

IMO this is the best procedure, and I insist on it due to (*).

bsdimp · 2024-05-23T19:26:26Z

Needing to do a make sysent is a magic step. There are, sadly, several other autogenerated files in our tree. I don't want to play whack-a-mole with automation to catch them all. I strongly believe it should be included, as a separate commit, in the review chain. Otherwise, we grow an ever increasing amount of DWIM when doing automation which we're already having trouble getting going. Ideally, we'd need to commit none of these files to the tree. Most of them could be autogenerated today, if we hacked the generation tool (I have a GSoC student working on this) which would lessen the issue. But today we do need the commits and we do need them to keep our github actions working.

I think the tradeoff of giving feedback to the submitter that certain things are broken (which we need to do more of, I think) and an 'all green' is worth the time to go through the process of landing out-weighs the ugliness in the review of having to skip over generated files.

bsdimp · 2024-05-23T19:29:44Z

(for this PR, though, I'm doing the make sysent and generating a commit to do the testing locally)

bsdimp · 2024-05-23T19:31:11Z

I also wonder how the cross builds are showing 'green' when they don't build for me natively w/o the make sysent.

…iptions Reviewed by: imp, kib Pull Request: freebsd#1248

Reviewed by: imp, kib Pull Request: freebsd#1248

This is needed to support Linux implementation which discards the leading slash when calling mq_open(2) Reviewed by: imp, kib Pull Request: freebsd#1248

Reviewed by: imp, kib Pull Request: freebsd#1248

bsdimp · 2024-05-23T20:11:04Z

Doh! my 'what to do when we lose the race' case of my automation failed to do the right thing, so this isn't showing up as merged.

kostikbel reviewed May 17, 2024

View reviewed changes

ricardobranco777 force-pushed the linux_mqueue branch 4 times, most recently from 0129a6e to 53c0352 Compare May 18, 2024 15:39

kostikbel approved these changes May 19, 2024

View reviewed changes