fspacectl: remove unneeded freebsd32 wrapper
ClosedPublic
Actions

Authored by brooks on Nov 15 2021, 6:15 PM.

Details

Reviewers

kib
khng
emaste
delphij

Group Reviewers

secteam

Commits

rGfea4a9aff120: fspacectl: remove unneeded freebsd32 wrapper

Summary

fspacectl(2) does not require special handling on freebsd32. The
presence of off_t in a struct does not cause it's size to change
between the native ABI and the 32-bit ABI supported by freebsd32
because off_t is always int64_t on BSD systems. Situations requiring
special handling may arise for i386 if and off_t is proceeded by an
8-byte aligned int, but that situation does not appaly. Further, byte
order only requires handling for paired argument or return registers.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 42801
Build 39689: arc lint + arc unit

Event Timeline

brooks created this revision.Nov 15 2021, 6:15 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 15 2021, 6:15 PM

brooks requested review of this revision.Nov 15 2021, 6:15 PM

Harbormaster completed remote builds in B42788: Diff 98526.Nov 15 2021, 6:15 PM

delphij accepted this revision.Nov 15 2021, 6:27 PM

This revision is now accepted and ready to land.Nov 15 2021, 6:27 PM

khng accepted this revision.Nov 15 2021, 6:35 PM

Well, this makes kern_fspacectl() useless and not providing the expectation for kern_* that arguments are already in the KVA, isn't it?

@khng points out that rqsr won't ever be NULL in kern_fspacectl. I was misled by the if (rqsr == NULL) that should have been a KASSERT (or nonexistent). I'll update the review shortly.

In D32994#745048, @kib wrote:

Well, this makes kern_fspacectl() useless and not providing the expectation for kern_* that arguments are already in the KVA, isn't it?

I'd prefer to keep kern_fspacectl() as I need it in CheriBSD. I will make it static so it will have zero cost in practice.

In D32994#745062, @brooks wrote:

In D32994#745048, @kib wrote:

Well, this makes kern_fspacectl() useless and not providing the expectation for kern_* that arguments are already in the KVA, isn't it?

I'd prefer to keep kern_fspacectl() as I need it in CheriBSD. I will make it static so it will have zero cost in practice.

I mean keep kern_, but do copyin/out in sys_.

In D32994#745067, @kib wrote:

In D32994#745062, @brooks wrote:

In D32994#745048, @kib wrote:

Well, this makes kern_fspacectl() useless and not providing the expectation for kern_* that arguments are already in the KVA, isn't it?

I'd prefer to keep kern_fspacectl() as I need it in CheriBSD. I will make it static so it will have zero cost in practice.

I mean keep kern_, but do copyin/out in sys_.

I don't think that's an expectation of kern_ functions. It's sometimes the desired interface to allow thunking (e.g. for linux or freebsd32), but it's not a consistent requirement.

In this case I'm not convinced we should be copying out unless we've called fo_fspacectl, but I could well be wrong there.

Given that I was wrong about the stack leak, I'd be fine dropping the diff to sys_generic.c entirely and leaving that part alone.

In D32994#745078, @brooks wrote:

I don't think that's an expectation of kern_ functions. It's sometimes the desired interface to allow thunking (e.g. for linux or freebsd32), but it's not a consistent requirement.

It is actually my plan to implement Linuxulator's fallocate(2) with a call to fspacectl as well.

Only remove freebsd32 bits

This revision now requires review to proceed.Nov 15 2021, 11:59 PM

Harbormaster completed remote builds in B42801: Diff 98557.Nov 15 2021, 11:59 PM

brooks edited the summary of this revision. (Show Details)Nov 15 2021, 11:59 PM

kib accepted this revision.Nov 16 2021, 12:16 AM

This revision is now accepted and ready to land.Nov 16 2021, 12:16 AM

khng accepted this revision.Nov 16 2021, 3:17 PM

delphij accepted this revision.Nov 16 2021, 9:49 PM

Situations requiring
special handling may arise for i386 if and off_t is proceeded by an
8-byte aligned int, but that situation does not appaly.

I was a bit confused at first by this statement - I think it is probably worth including, but maybe make it stand apart from the main part of the message or make it explicitly parenthetical or so?

Fixes: 0dc332bff200c

Closed by commit rGfea4a9aff120: fspacectl: remove unneeded freebsd32 wrapper (authored by brooks). · Explain WhyNov 18 2021, 1:02 AM

This revision was automatically updated to reflect the committed changes.

brooks added a commit: rGfea4a9aff120: fspacectl: remove unneeded freebsd32 wrapper.