Page MenuHomeFreeBSD
Differential D26784

vmapbuf: don't smuggle user address via b_data
ClosedPublic
Actions

Authored by brooks on Oct 14 2020, 11:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 13, 12:03 PM
Unknown Object (File)
Fri, Apr 11, 11:51 AM
Unknown Object (File)
Mar 5 2025, 9:21 AM
Unknown Object (File)
Feb 18 2025, 6:00 AM
Unknown Object (File)
Feb 15 2025, 6:44 PM
Unknown Object (File)
Feb 15 2025, 6:32 PM
Unknown Object (File)
Feb 15 2025, 6:24 PM
Unknown Object (File)
Feb 15 2025, 6:15 PM
View All Files
Subscribers
dab

Details

Reviewers
imp
scottl
jhb
Group Reviewers
cam
Commits
rS367145: MFC r366911:
rS367140: MFC r366911:
rS366911: vmapbuf: don't smuggle address or length in buf
Summary

Instead, add a uaddr argument to vmapbuf. Since this argument is
always a pointer use a type of void * and cast to vm_offset_t in
vmapbuf. (In CheriBSD we've altered vm_fault_quick_hold_pages to
take a pointer and check its bounds.)

In no other situtation does b_data contain a user pointer and vmapbuf
replaces b_data with the actual mapping.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34258
Build 31401: arc lint + arc unit

Event Timeline

brooks created this revision.Oct 14 2020, 11:53 PM
Herald added a reviewer: cam. · View Herald TranscriptOct 14 2020, 11:53 PM
Herald added a subscriber: dab. · View Herald Transcript
brooks requested review of this revision.Oct 14 2020, 11:53 PM
Harbormaster completed remote builds in B34172: Diff 78238.Oct 14 2020, 11:53 PM
brooks added a comment.Oct 15 2020, 5:35 PM

CI run: https://cirrus-ci.com/task/5059292485124096

jhb added a comment.Oct 15 2020, 9:56 PM

I know I didn't say this earlier in CheriBSD, but looking at the patch again, I think it might be cleaner to pass in the size explicitly as well and have vmapbuf set bp->bufsize rather than than smuggling that in as well. Together, b_data, b_offset, and b_bufsize describe the memory buffer, and passing in the length would let vmapbuf be responsible for setting all of those.

sys/ufs/ffs/ffs_rawread.c
249

Unrelated bug: vmapbuf() overwrites bp->b_offset. This should only be setting bp->b_iooffset.

brooks updated this revision to Diff 78430.Oct 19 2020, 3:09 PM
  • Pass size rather than smuggling via b_bufsize.
Harbormaster completed remote builds in B34258: Diff 78430.Oct 19 2020, 3:09 PM
brooks added a comment.Oct 19 2020, 5:20 PM

https://cirrus-ci.com/task/5974398819631104

jhb accepted this revision.Oct 19 2020, 8:25 PM

Thanks.

sys/kern/vfs_bio.c
4915

This check is now dead code that can be removed I think since size_t is unsigned?

This revision is now accepted and ready to land.Oct 19 2020, 8:25 PM
brooks updated this revision to Diff 78470.Oct 19 2020, 9:06 PM
  • GC a dead check.
This revision now requires review to proceed.Oct 19 2020, 9:06 PM
Harbormaster completed remote builds in B34267: Diff 78470.Oct 19 2020, 9:06 PM
imp accepted this revision.Oct 19 2020, 11:26 PM

This is good... only caveat is that MFC code near this stuff might be a pain... A minor inconvenience at best, so this is a LGTM!

This revision is now accepted and ready to land.Oct 19 2020, 11:26 PM
Closed by commit rS366911: vmapbuf: don't smuggle address or length in buf (authored by brooks). · Explain WhyOct 21 2020, 4:00 PM
This revision was automatically updated to reflect the committed changes.
brooks added a commit: rS366911: vmapbuf: don't smuggle address or length in buf.
brooks added a commit: rS367140: MFC r366911:.Oct 29 2020, 6:29 PM
brooks added a commit: rS367145: MFC r366911:.Oct 29 2020, 10:00 PM

Revision Contents
Changeset List

PathSize
sys/
cam/
8 lines
dev/
nvme/
4 lines
kern/
9 lines
sys/
2 lines
ufs/
ffs/
7 lines

Diff 78430

View Options

sys/cam/cam_periph.c

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h" #include "opt_vm.h"
#include <sys/param.h> #include <sys/param.h>
#include <sys/bio.h> #include <sys/bio.h>
#include <sys/blist.h> #include <sys/blist.h>
#include <sys/buf.h> #include <sys/buf.h>
#include <sys/conf.h> #include <sys/conf.h>
#include <sys/disk.h> #include <sys/disk.h>
#include <sys/disklabel.h>
#include <sys/eventhandler.h> #include <sys/eventhandler.h>
#include <sys/fcntl.h> #include <sys/fcntl.h>
#include <sys/lock.h> #include <sys/lock.h>
#include <sys/kernel.h> #include <sys/kernel.h>
#include <sys/mount.h> #include <sys/mount.h>
#include <sys/namei.h> #include <sys/namei.h>
#include <sys/malloc.h> #include <sys/malloc.h>
#include <sys/pctrie.h> #include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Linesswaponsomething(struct vnode *vp, void *id, u_long nblks,
sp->sw_nblks = nblks; sp->sw_nblks = nblks;
sp->sw_used = 0; sp->sw_used = 0;
sp->sw_strategy = strategy; sp->sw_strategy = strategy;
sp->sw_close = close; sp->sw_close = close;
sp->sw_flags = flags; sp->sw_flags = flags;
sp->sw_blist = blist_create(nblks, M_WAITOK); sp->sw_blist = blist_create(nblks, M_WAITOK);
/* /*
* Do not free the first two block in order to avoid overwriting * Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition * any bsd label at the front of the partition
*/ */
blist_free(sp->sw_blist, 2, nblks - 2); blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
nblks - howmany(BBSIZE, PAGE_SIZE));
alcUnsubmitted
Not Done
Inline Actions

Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing."

alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0; dvbase = 0;
mtx_lock(&sw_dev_mtx); mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) { TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) { if (tsp->sw_end >= dvbase) {
/* /*
* We put one uncovered page between the devices * We put one uncovered page between the devices
* in order to definitively prevent any cross-device * in order to definitively prevent any cross-device
* I/O requests * I/O requests
*/ */
dvbase = tsp->sw_end + 1; dvbase = tsp->sw_end + 1;
} }
} }
sp->sw_first = dvbase; sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks; sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list); TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++; nswapdev++;
swap_pager_avail += nblks - 2; swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
alcUnsubmitted
Not Done
Inline Actions

This should also use howmany(BBSIZE, PAGE_SIZE).

alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks; swap_total += nblks;
swapon_check_swzone(); swapon_check_swzone();
swp_sizecheck(); swp_sizecheck();
mtx_unlock(&sw_dev_mtx); mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp); EVENTHANDLER_INVOKE(swapon, sp);
} }
/* /*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines
View Options

sys/dev/nvme/nvme_ctrlr.c

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h" #include "opt_vm.h"
#include <sys/param.h> #include <sys/param.h>
#include <sys/bio.h> #include <sys/bio.h>
#include <sys/blist.h> #include <sys/blist.h>
#include <sys/buf.h> #include <sys/buf.h>
#include <sys/conf.h> #include <sys/conf.h>
#include <sys/disk.h> #include <sys/disk.h>
#include <sys/disklabel.h>
#include <sys/eventhandler.h> #include <sys/eventhandler.h>
#include <sys/fcntl.h> #include <sys/fcntl.h>
#include <sys/lock.h> #include <sys/lock.h>
#include <sys/kernel.h> #include <sys/kernel.h>
#include <sys/mount.h> #include <sys/mount.h>
#include <sys/namei.h> #include <sys/namei.h>
#include <sys/malloc.h> #include <sys/malloc.h>
#include <sys/pctrie.h> #include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Linesswaponsomething(struct vnode *vp, void *id, u_long nblks,
sp->sw_nblks = nblks; sp->sw_nblks = nblks;
sp->sw_used = 0; sp->sw_used = 0;
sp->sw_strategy = strategy; sp->sw_strategy = strategy;
sp->sw_close = close; sp->sw_close = close;
sp->sw_flags = flags; sp->sw_flags = flags;
sp->sw_blist = blist_create(nblks, M_WAITOK); sp->sw_blist = blist_create(nblks, M_WAITOK);
/* /*
* Do not free the first two block in order to avoid overwriting * Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition * any bsd label at the front of the partition
*/ */
blist_free(sp->sw_blist, 2, nblks - 2); blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
nblks - howmany(BBSIZE, PAGE_SIZE));
alcUnsubmitted
Not Done
Inline Actions

Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing."

alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0; dvbase = 0;
mtx_lock(&sw_dev_mtx); mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) { TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) { if (tsp->sw_end >= dvbase) {
/* /*
* We put one uncovered page between the devices * We put one uncovered page between the devices
* in order to definitively prevent any cross-device * in order to definitively prevent any cross-device
* I/O requests * I/O requests
*/ */
dvbase = tsp->sw_end + 1; dvbase = tsp->sw_end + 1;
} }
} }
sp->sw_first = dvbase; sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks; sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list); TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++; nswapdev++;
swap_pager_avail += nblks - 2; swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
alcUnsubmitted
Not Done
Inline Actions

This should also use howmany(BBSIZE, PAGE_SIZE).

alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks; swap_total += nblks;
swapon_check_swzone(); swapon_check_swzone();
swp_sizecheck(); swp_sizecheck();
mtx_unlock(&sw_dev_mtx); mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp); EVENTHANDLER_INVOKE(swapon, sp);
} }
/* /*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines
View Options

sys/kern/vfs_bio.c

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h" #include "opt_vm.h"
#include <sys/param.h> #include <sys/param.h>
#include <sys/bio.h> #include <sys/bio.h>
#include <sys/blist.h> #include <sys/blist.h>
#include <sys/buf.h> #include <sys/buf.h>
#include <sys/conf.h> #include <sys/conf.h>
#include <sys/disk.h> #include <sys/disk.h>
#include <sys/disklabel.h>
#include <sys/eventhandler.h> #include <sys/eventhandler.h>
#include <sys/fcntl.h> #include <sys/fcntl.h>
#include <sys/lock.h> #include <sys/lock.h>
#include <sys/kernel.h> #include <sys/kernel.h>
#include <sys/mount.h> #include <sys/mount.h>
#include <sys/namei.h> #include <sys/namei.h>
#include <sys/malloc.h> #include <sys/malloc.h>
#include <sys/pctrie.h> #include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Linesswaponsomething(struct vnode *vp, void *id, u_long nblks,
sp->sw_nblks = nblks; sp->sw_nblks = nblks;
sp->sw_used = 0; sp->sw_used = 0;
sp->sw_strategy = strategy; sp->sw_strategy = strategy;
sp->sw_close = close; sp->sw_close = close;
sp->sw_flags = flags; sp->sw_flags = flags;
sp->sw_blist = blist_create(nblks, M_WAITOK); sp->sw_blist = blist_create(nblks, M_WAITOK);
/* /*
* Do not free the first two block in order to avoid overwriting * Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition * any bsd label at the front of the partition
*/ */
blist_free(sp->sw_blist, 2, nblks - 2); blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
nblks - howmany(BBSIZE, PAGE_SIZE));
alcUnsubmitted
Not Done
Inline Actions

Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing."

alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0; dvbase = 0;
mtx_lock(&sw_dev_mtx); mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) { TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) { if (tsp->sw_end >= dvbase) {
/* /*
* We put one uncovered page between the devices * We put one uncovered page between the devices
* in order to definitively prevent any cross-device * in order to definitively prevent any cross-device
* I/O requests * I/O requests
*/ */
dvbase = tsp->sw_end + 1; dvbase = tsp->sw_end + 1;
} }
} }
sp->sw_first = dvbase; sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks; sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list); TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++; nswapdev++;
swap_pager_avail += nblks - 2; swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
alcUnsubmitted
Not Done
Inline Actions

This should also use howmany(BBSIZE, PAGE_SIZE).

alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks; swap_total += nblks;
swapon_check_swzone(); swapon_check_swzone();
swp_sizecheck(); swp_sizecheck();
mtx_unlock(&sw_dev_mtx); mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp); EVENTHANDLER_INVOKE(swapon, sp);
} }
/* /*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines
View Options

sys/sys/buf.h

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h" #include "opt_vm.h"
#include <sys/param.h> #include <sys/param.h>
#include <sys/bio.h> #include <sys/bio.h>
#include <sys/blist.h> #include <sys/blist.h>
#include <sys/buf.h> #include <sys/buf.h>
#include <sys/conf.h> #include <sys/conf.h>
#include <sys/disk.h> #include <sys/disk.h>
#include <sys/disklabel.h>
#include <sys/eventhandler.h> #include <sys/eventhandler.h>
#include <sys/fcntl.h> #include <sys/fcntl.h>
#include <sys/lock.h> #include <sys/lock.h>
#include <sys/kernel.h> #include <sys/kernel.h>
#include <sys/mount.h> #include <sys/mount.h>
#include <sys/namei.h> #include <sys/namei.h>
#include <sys/malloc.h> #include <sys/malloc.h>
#include <sys/pctrie.h> #include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Linesswaponsomething(struct vnode *vp, void *id, u_long nblks,
sp->sw_nblks = nblks; sp->sw_nblks = nblks;
sp->sw_used = 0; sp->sw_used = 0;
sp->sw_strategy = strategy; sp->sw_strategy = strategy;
sp->sw_close = close; sp->sw_close = close;
sp->sw_flags = flags; sp->sw_flags = flags;
sp->sw_blist = blist_create(nblks, M_WAITOK); sp->sw_blist = blist_create(nblks, M_WAITOK);
/* /*
* Do not free the first two block in order to avoid overwriting * Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition * any bsd label at the front of the partition
*/ */
blist_free(sp->sw_blist, 2, nblks - 2); blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
nblks - howmany(BBSIZE, PAGE_SIZE));
alcUnsubmitted
Not Done
Inline Actions

Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing."

alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0; dvbase = 0;
mtx_lock(&sw_dev_mtx); mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) { TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) { if (tsp->sw_end >= dvbase) {
/* /*
* We put one uncovered page between the devices * We put one uncovered page between the devices
* in order to definitively prevent any cross-device * in order to definitively prevent any cross-device
* I/O requests * I/O requests
*/ */
dvbase = tsp->sw_end + 1; dvbase = tsp->sw_end + 1;
} }
} }
sp->sw_first = dvbase; sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks; sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list); TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++; nswapdev++;
swap_pager_avail += nblks - 2; swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
alcUnsubmitted
Not Done
Inline Actions

This should also use howmany(BBSIZE, PAGE_SIZE).

alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks; swap_total += nblks;
swapon_check_swzone(); swapon_check_swzone();
swp_sizecheck(); swp_sizecheck();
mtx_unlock(&sw_dev_mtx); mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp); EVENTHANDLER_INVOKE(swapon, sp);
} }
/* /*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines
View Options

sys/ufs/ffs/ffs_rawread.c

Loading...