Page MenuHomeFreeBSD
Differential D34190

file: Make fget*() and getvnode*() consistent about initializing *fpp
ClosedPublic
Actions

Authored by markj on Feb 7 2022, 3:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 20, 8:41 AM
Unknown Object (File)
Feb 23 2025, 5:55 AM
Unknown Object (File)
Feb 21 2025, 6:52 PM
Unknown Object (File)
Feb 20 2025, 12:46 AM
Unknown Object (File)
Feb 2 2025, 11:14 AM
Unknown Object (File)
Jan 28 2025, 2:21 PM
Unknown Object (File)
Jan 17 2025, 3:26 AM
Unknown Object (File)
Jan 16 2025, 9:46 AM
View All 58 Files
Subscribers
imp

Details

Reviewers
kib
mjg
Commits
rG300cfb96fc22: file: Make fget*() and getvnode*() consistent about initializing *fpp
Summary

Most fget*() functions initialize the output parameter to NULL. Make them
all behave consistently. This fixes at least one bug in a consumer,
_filemon_wrapper_openat().

Reported by: syzbot+01c0459408f896a5933a@syzkaller.appspotmail.com

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 44320
Build 41208: arc lint + arc unit

Event Timeline

markj created this revision.Feb 7 2022, 3:19 PM
Herald added a subscriber: imp. · View Herald TranscriptFeb 7 2022, 3:19 PM
markj requested review of this revision.Feb 7 2022, 3:19 PM
Harbormaster completed remote builds in B44319: Diff 102456.Feb 7 2022, 3:19 PM
kib added inline comments.Feb 7 2022, 3:23 PM
sys/kern/vfs_syscalls.c
4322

Should this return handled?

markj added inline comments.Feb 7 2022, 3:26 PM
sys/kern/vfs_syscalls.c
4322

fget_unlocked() modifies *fpp only when it returns success. I think it's ok to rely on that here. This diff is just extending that semantic to getvnode()/getvnode_path().

kib accepted this revision.Feb 7 2022, 3:39 PM
kib added inline comments.
sys/kern/vfs_syscalls.c
4322

My reaction is to the fact that 'modifying only on success' != 'setting to NULL on failure'. It is somewhat confusing for getvnode() now.

This revision is now accepted and ready to land.Feb 7 2022, 3:39 PM
markj added inline comments.Feb 7 2022, 4:53 PM
sys/kern/vfs_syscalls.c
4322

Hmm. fget() explicitly initializes *fpp = NULL, but fget_unlocked() does not. This results in inconsistencies: fget_cap() may or may not initialize *fpp = NULL in the error case depending on whether CAPABILITIES is defined.

IMO fget_unlocked() should change as well.

markj updated this revision to Diff 102458.Feb 7 2022, 5:43 PM
  • Make fget_unlocked_seq() private to kern_descrip.c
  • Make fget* consistent about initializing the output pointer. Some routines were doing it, some not, but it seems preferable to provide some guarantees there to help ensure that buggy callers fail closed.
This revision now requires review to proceed.Feb 7 2022, 5:43 PM
Harbormaster completed remote builds in B44320: Diff 102458.Feb 7 2022, 5:43 PM
markj retitled this revision from file: Make getvnode*() set the returned file handle to NULL upon error to file: Make fget*() and getvnode*() consistent about initializing *fpp.Feb 7 2022, 5:44 PM
markj edited the summary of this revision. (Show Details)
markj added a reviewer: mjg.
kib accepted this revision.Feb 7 2022, 7:06 PM
This revision is now accepted and ready to land.Feb 7 2022, 7:06 PM
Closed by commit rG300cfb96fc22: file: Make fget*() and getvnode*() consistent about initializing *fpp (authored by markj). · Explain WhyFeb 8 2022, 6:37 PM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rG300cfb96fc22: file: Make fget*() and getvnode*() consistent about initializing *fpp.
mjg added a comment.Feb 8 2022, 9:05 PM

I see I'm late. Just a remark that this nullifying is pretty weird and thing to do instead is to set the pointer to a poisoned value when running with invariants, while patching all the consumers to not look at it if an error was returned. Maybe I'll get around to it.

Revision Contents
Changeset List

PathSize
sys/
kern/
12 lines
2 lines
sys/
3 lines

Diff 102458

View Options

sys/kern/kern_descrip.c

Show First 20 Lines • Show All 309 Lines • ▼ Show 20 Linesg_dev_getprovider(struct cdev *dev)
cp = dev->si_drv2; cp = dev->si_drv2;
return (cp->provider); return (cp->provider);
} }
static struct g_geom * static struct g_geom *
g_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused) g_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused)
{ {
struct g_geom *gp; struct g_geom *gp;
struct g_geom_alias *gap;
struct g_consumer *cp; struct g_consumer *cp;
struct g_dev_softc *sc; struct g_dev_softc *sc;
int error; int error;
struct cdev *dev; struct cdev *dev, *adev;
char buf[SPECNAMELEN + 6]; char buf[SPECNAMELEN + 6];
g_trace(G_T_TOPOLOGY, "dev_taste(%s,%s)", mp->name, pp->name); g_trace(G_T_TOPOLOGY, "dev_taste(%s,%s)", mp->name, pp->name);
g_topology_assert(); g_topology_assert();
gp = g_new_geomf(mp, "%s", pp->name); gp = g_new_geomf(mp, "%s", pp->name);
sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO); sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO);
mtx_init(&sc->sc_mtx, "g_dev", NULL, MTX_DEF); mtx_init(&sc->sc_mtx, "g_dev", NULL, MTX_DEF);
cp = g_new_consumer(gp); cp = g_new_consumer(gp);
Show All 22 Linesg_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused)
error = init_dumpdev(dev); error = init_dumpdev(dev);
if (error != 0) if (error != 0)
printf("%s: init_dumpdev() failed (gp->name=%s, error=%d)\n", printf("%s: init_dumpdev() failed (gp->name=%s, error=%d)\n",
__func__, gp->name, error); __func__, gp->name, error);
g_dev_attrchanged(cp, "GEOM::physpath"); g_dev_attrchanged(cp, "GEOM::physpath");
snprintf(buf, sizeof(buf), "cdev=%s", gp->name); snprintf(buf, sizeof(buf), "cdev=%s", gp->name);
devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK); devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK);
/*
* Now add all the aliases for this drive
*/
LIST_FOREACH(gap, &pp->geom->aliases, ga_next) {
error = make_dev_alias_p(MAKEDEV_CHECKNAME | MAKEDEV_WAITOK, &adev, dev,
"%s", gap->ga_alias);
if (error) {
printf("%s: make_dev_alias_p() failed (name=%s, error=%d)\n",
__func__, gap->ga_alias, error);
continue;
}
adev->si_flags |= SI_UNMAPPED;
adev->si_iosize_max = dev->si_iosize_max;
adev->si_drv2 = dev->si_drv2;
cemUnsubmitted
Not Done
Inline Actions

Could any of this stuff be moved inside make_dev_alias_p?

cem: Could any of this stuff be moved inside `make_dev_alias_p`?
impAuthorUnsubmitted
Not Done
Inline Actions

I don't think so. There's other places in the tree that don't do these things.

imp: I don't think so. There's other places in the tree that don't do these things.
snprintf(buf, sizeof(buf), "cdev=%s", gap->ga_alias);
devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK);
}
return (gp); return (gp);
} }
static int static int
g_dev_open(struct cdev *dev, int flags, int fmt, struct thread *td) g_dev_open(struct cdev *dev, int flags, int fmt, struct thread *td)
{ {
struct g_consumer *cp; struct g_consumer *cp;
▲ Show 20 Lines • Show All 459 Lines • Show Last 20 Lines
View Options

sys/kern/vfs_syscalls.c

Show First 20 Lines • Show All 309 Lines • ▼ Show 20 Linesg_dev_getprovider(struct cdev *dev)
cp = dev->si_drv2; cp = dev->si_drv2;
return (cp->provider); return (cp->provider);
} }
static struct g_geom * static struct g_geom *
g_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused) g_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused)
{ {
struct g_geom *gp; struct g_geom *gp;
struct g_geom_alias *gap;
struct g_consumer *cp; struct g_consumer *cp;
struct g_dev_softc *sc; struct g_dev_softc *sc;
int error; int error;
struct cdev *dev; struct cdev *dev, *adev;
char buf[SPECNAMELEN + 6]; char buf[SPECNAMELEN + 6];
g_trace(G_T_TOPOLOGY, "dev_taste(%s,%s)", mp->name, pp->name); g_trace(G_T_TOPOLOGY, "dev_taste(%s,%s)", mp->name, pp->name);
g_topology_assert(); g_topology_assert();
gp = g_new_geomf(mp, "%s", pp->name); gp = g_new_geomf(mp, "%s", pp->name);
sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO); sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO);
mtx_init(&sc->sc_mtx, "g_dev", NULL, MTX_DEF); mtx_init(&sc->sc_mtx, "g_dev", NULL, MTX_DEF);
cp = g_new_consumer(gp); cp = g_new_consumer(gp);
Show All 22 Linesg_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused)
error = init_dumpdev(dev); error = init_dumpdev(dev);
if (error != 0) if (error != 0)
printf("%s: init_dumpdev() failed (gp->name=%s, error=%d)\n", printf("%s: init_dumpdev() failed (gp->name=%s, error=%d)\n",
__func__, gp->name, error); __func__, gp->name, error);
g_dev_attrchanged(cp, "GEOM::physpath"); g_dev_attrchanged(cp, "GEOM::physpath");
snprintf(buf, sizeof(buf), "cdev=%s", gp->name); snprintf(buf, sizeof(buf), "cdev=%s", gp->name);
devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK); devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK);
/*
* Now add all the aliases for this drive
*/
LIST_FOREACH(gap, &pp->geom->aliases, ga_next) {
error = make_dev_alias_p(MAKEDEV_CHECKNAME | MAKEDEV_WAITOK, &adev, dev,
"%s", gap->ga_alias);
if (error) {
printf("%s: make_dev_alias_p() failed (name=%s, error=%d)\n",
__func__, gap->ga_alias, error);
continue;
}
adev->si_flags |= SI_UNMAPPED;
adev->si_iosize_max = dev->si_iosize_max;
adev->si_drv2 = dev->si_drv2;
cemUnsubmitted
Not Done
Inline Actions

Could any of this stuff be moved inside make_dev_alias_p?

cem: Could any of this stuff be moved inside `make_dev_alias_p`?
impAuthorUnsubmitted
Not Done
Inline Actions

I don't think so. There's other places in the tree that don't do these things.

imp: I don't think so. There's other places in the tree that don't do these things.
snprintf(buf, sizeof(buf), "cdev=%s", gap->ga_alias);
devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK);
}
return (gp); return (gp);
} }
static int static int
g_dev_open(struct cdev *dev, int flags, int fmt, struct thread *td) g_dev_open(struct cdev *dev, int flags, int fmt, struct thread *td)
{ {
struct g_consumer *cp; struct g_consumer *cp;
▲ Show 20 Lines • Show All 459 Lines • Show Last 20 Lines
View Options

sys/sys/filedesc.h

Show First 20 Lines • Show All 309 Lines • ▼ Show 20 Linesg_dev_getprovider(struct cdev *dev)
cp = dev->si_drv2; cp = dev->si_drv2;
return (cp->provider); return (cp->provider);
} }
static struct g_geom * static struct g_geom *
g_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused) g_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused)
{ {
struct g_geom *gp; struct g_geom *gp;
struct g_geom_alias *gap;
struct g_consumer *cp; struct g_consumer *cp;
struct g_dev_softc *sc; struct g_dev_softc *sc;
int error; int error;
struct cdev *dev; struct cdev *dev, *adev;
char buf[SPECNAMELEN + 6]; char buf[SPECNAMELEN + 6];
g_trace(G_T_TOPOLOGY, "dev_taste(%s,%s)", mp->name, pp->name); g_trace(G_T_TOPOLOGY, "dev_taste(%s,%s)", mp->name, pp->name);
g_topology_assert(); g_topology_assert();
gp = g_new_geomf(mp, "%s", pp->name); gp = g_new_geomf(mp, "%s", pp->name);
sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO); sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO);
mtx_init(&sc->sc_mtx, "g_dev", NULL, MTX_DEF); mtx_init(&sc->sc_mtx, "g_dev", NULL, MTX_DEF);
cp = g_new_consumer(gp); cp = g_new_consumer(gp);
Show All 22 Linesg_dev_taste(struct g_class *mp, struct g_provider *pp, int insist __unused)
error = init_dumpdev(dev); error = init_dumpdev(dev);
if (error != 0) if (error != 0)
printf("%s: init_dumpdev() failed (gp->name=%s, error=%d)\n", printf("%s: init_dumpdev() failed (gp->name=%s, error=%d)\n",
__func__, gp->name, error); __func__, gp->name, error);
g_dev_attrchanged(cp, "GEOM::physpath"); g_dev_attrchanged(cp, "GEOM::physpath");
snprintf(buf, sizeof(buf), "cdev=%s", gp->name); snprintf(buf, sizeof(buf), "cdev=%s", gp->name);
devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK); devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK);
/*
* Now add all the aliases for this drive
*/
LIST_FOREACH(gap, &pp->geom->aliases, ga_next) {
error = make_dev_alias_p(MAKEDEV_CHECKNAME | MAKEDEV_WAITOK, &adev, dev,
"%s", gap->ga_alias);
if (error) {
printf("%s: make_dev_alias_p() failed (name=%s, error=%d)\n",
__func__, gap->ga_alias, error);
continue;
}
adev->si_flags |= SI_UNMAPPED;
adev->si_iosize_max = dev->si_iosize_max;
adev->si_drv2 = dev->si_drv2;
cemUnsubmitted
Not Done
Inline Actions

Could any of this stuff be moved inside make_dev_alias_p?

cem: Could any of this stuff be moved inside `make_dev_alias_p`?
impAuthorUnsubmitted
Not Done
Inline Actions

I don't think so. There's other places in the tree that don't do these things.

imp: I don't think so. There's other places in the tree that don't do these things.
snprintf(buf, sizeof(buf), "cdev=%s", gap->ga_alias);
devctl_notify_f("GEOM", "DEV", "CREATE", buf, M_WAITOK);
}
return (gp); return (gp);
} }
static int static int
g_dev_open(struct cdev *dev, int flags, int fmt, struct thread *td) g_dev_open(struct cdev *dev, int flags, int fmt, struct thread *td)
{ {
struct g_consumer *cp; struct g_consumer *cp;
▲ Show 20 Lines • Show All 459 Lines • Show Last 20 Lines