Author: kib Date: Thu Jan 7 20:08:02 2016 New Revision: 293346 URL: https://svnweb.freebsd.org/changeset/base/293346
Log: Provide yet another KPI for cdev creation, make_dev_s(9). Immediate problem fixed by the new KPI is the long-standing race between device creation and assignments to cdev->si_drv1 and cdev->si_drv2, which allows the window where cdevsw methods might be called with si_drv1,2 fields not yet set. Devices typically checked for NULL and returned spurious errors to usermode, and often left some methods unchecked. The new function interface is designed to be extensible, which should allow to add more features to make_dev_s(9) without inventing yet another name for function to create devices, while maintaining KPI and even KBI backward-compatibility. Reviewed by: hps, jhb Sponsored by: The FreeBSD Foundation MFC after: 3 weeks Differential revision: https://reviews.freebsd.org/D4746 Modified: head/share/man/man9/Makefile head/share/man/man9/make_dev.9 head/sys/kern/kern_conf.c head/sys/sys/conf.h Modified: head/share/man/man9/Makefile ============================================================================== --- head/share/man/man9/Makefile Thu Jan 7 19:58:23 2016 (r293345) +++ head/share/man/man9/Makefile Thu Jan 7 20:08:02 2016 (r293346) @@ -1013,7 +1013,8 @@ MLINKS+=make_dev.9 destroy_dev.9 \ make_dev.9 make_dev_alias_p.9 \ make_dev.9 make_dev_cred.9 \ make_dev.9 make_dev_credf.9 \ - make_dev.9 make_dev_p.9 + make_dev.9 make_dev_p.9 \ + make_dev.9 make_dev_s.9 MLINKS+=malloc.9 free.9 \ malloc.9 MALLOC_DECLARE.9 \ malloc.9 MALLOC_DEFINE.9 \ Modified: head/share/man/man9/make_dev.9 ============================================================================== --- head/share/man/man9/make_dev.9 Thu Jan 7 19:58:23 2016 (r293345) +++ head/share/man/man9/make_dev.9 Thu Jan 7 20:08:02 2016 (r293346) @@ -24,7 +24,7 @@ .\" .\" $FreeBSD$ .\" -.Dd Dec 22, 2012 +.Dd Jan 3, 2016 .Dt MAKE_DEV 9 .Os .Sh NAME @@ -32,6 +32,7 @@ .Nm make_dev_cred , .Nm make_dev_credf , .Nm make_dev_p , +.Nm make_dev_s , .Nm make_dev_alias , .Nm make_dev_alias_p , .Nm destroy_dev , @@ -45,16 +46,10 @@ and DEVFS registration for devices .Sh SYNOPSIS .In sys/param.h .In sys/conf.h -.Ft struct cdev * -.Fn make_dev "struct cdevsw *cdevsw" "int unit" "uid_t uid" "gid_t gid" "int perms" "const char *fmt" ... -.Ft struct cdev * -.Fn make_dev_cred "struct cdevsw *cdevsw" "int unit" "struct ucred *cr" "uid_t uid" "gid_t gid" "int perms" "const char *fmt" ... -.Ft struct cdev * -.Fn make_dev_credf "int flags" "struct cdevsw *cdevsw" "int unit" "struct ucred *cr" "uid_t uid" "gid_t gid" "int perms" "const char *fmt" ... +.Ft void +.Fn make_dev_args_init "struct make_dev_args *args" .Ft int -.Fn make_dev_p "int flags" "struct cdev **cdev" "struct cdevsw *devsw" "struct ucred *cr" "uid_t uid" "gid_t gid" "int mode" "const char *fmt" ... -.Ft struct cdev * -.Fn make_dev_alias "struct cdev *pdev" "const char *fmt" ... +.Fn make_dev_s "struct make_dev_args *args" "struct cdev **cdev" "const char *fmt" ... .Ft int .Fn make_dev_alias_p "int flags" "struct cdev **cdev" "struct cdev *pdev" "const char *fmt" ... .Ft void @@ -67,12 +62,26 @@ and DEVFS registration for devices .Fn destroy_dev_drain "struct cdevsw *csw" .Ft void .Fn dev_depends "struct cdev *pdev" "struct cdev *cdev" +.Pp +LEGACY INTERFACES +.Ft struct cdev * +.Fn make_dev "struct cdevsw *cdevsw" "int unit" "uid_t uid" "gid_t gid" "int perms" "const char *fmt" ... +.Ft struct cdev * +.Fn make_dev_cred "struct cdevsw *cdevsw" "int unit" "struct ucred *cr" "uid_t uid" "gid_t gid" "int perms" "const char *fmt" ... +.Ft struct cdev * +.Fn make_dev_credf "int flags" "struct cdevsw *cdevsw" "int unit" "struct ucred *cr" "uid_t uid" "gid_t gid" "int perms" "const char *fmt" ... +.Ft int +.Fn make_dev_p "int flags" "struct cdev **cdev" "struct cdevsw *devsw" "struct ucred *cr" "uid_t uid" "gid_t gid" "int mode" "const char *fmt" ... +.Ft struct cdev * +.Fn make_dev_alias "struct cdev *pdev" "const char *fmt" ... .Sh DESCRIPTION The -.Fn make_dev_credf +.Fn make_dev_s function creates a .Fa cdev -structure for a new device. +structure for a new device, which is returned into the +.Fa cdev +argument. It also notifies .Xr devfs 5 of the presence of the new device, that causes corresponding nodes @@ -80,10 +89,34 @@ to be created. Besides this, a .Xr devctl 4 notification is sent. -The device will be owned by -.Va uid , +The function takes the structure +.Va struct make_dev_args args , +which specifies the parameters for the device creation: +.Pp +.Bd -literal -offset indent -compact +struct make_dev_args { + size_t mda_size; + int mda_flags; + struct cdevsw *mda_devsw; + struct ucred *mda_cr; + uid_t mda_uid; + gid_t mda_gid; + int mda_mode; + int mda_unit; + void *mda_si_drv1; + void *mda_si_drv2; +}; +.Ed +Before use and filling with the desired values, the structure must be +initialized by the +.Fn make_dev_args_init +function, which ensures that future kernel interface expansion does +not affect driver source code or binary interface. +.Pp +The created device will be owned by +.Va args.mda_uid , with the group ownership as -.Va gid . +.Va args.mda_gid . The name is the expansion of .Va fmt and following arguments as @@ -97,7 +130,7 @@ mount point and may contain slash .Ql / characters to denote subdirectories. The permissions of the file specified in -.Va perms +.Va args.mda_mode are defined in .In sys/stat.h : .Pp @@ -126,29 +159,28 @@ are defined in .Ed .Pp The -.Va cr +.Va args.mda_cr argument specifies credentials that will be stored in the .Fa si_cred member of the initialized .Fa struct cdev . +.Pp The -.Va flags +.Va args.mda_flags argument alters the operation of -.Fn make_dev_credf -or -.Fn make_dev_p . +.Fn make_dev_s. The following values are currently accepted: .Pp -.Bl -tag -width "MAKEDEV_CHECKNAME" -compact -offset indent -.It MAKEDEV_REF +.Bl -tag -width "It Dv MAKEDEV_CHECKNAME" -compact -offset indent +.It Dv MAKEDEV_REF reference the created device -.It MAKEDEV_NOWAIT +.It Dv MAKEDEV_NOWAIT do not sleep, the call may fail -.It MAKEDEV_WAITOK +.It Dv MAKEDEV_WAITOK allow the function to sleep to satisfy malloc -.It MAKEDEV_ETERNAL +.It Dv MAKEDEV_ETERNAL created device will be never destroyed -.It MAKEDEV_CHECKNAME +.It Dv MAKEDEV_CHECKNAME return an error if the device name is invalid or already exists .El .Pp @@ -189,10 +221,49 @@ For the convenience, use the flag for the code that can be compiled into kernel or loaded (and unloaded) as loadable module. .Pp -A panic will occur if the MAKEDEV_CHECKNAME flag is not specified +A panic will occur if the +.Dv MAKEDEV_CHECKNAME +flag is not specified and the device name is invalid or already exists. .Pp The +.Fn make_dev_p +use of the form +.Bd -literal -offset indent +struct cdev *dev; +int res; +res = make_dev_p(flags, &dev, cdevsw, cred, uid, gid, perms, name); +.Ed +is equivalent to the code +.Bd -literal -offset indent +struct cdev *dev; +struct make_dev_args args; +int res; + +make_dev_args_init(&args); +args.mda_flags = flags; +args.mda_devsw = cdevsw; +args.mda_cred = cred; +args.mda_uid = uid; +args.mda_gid = gid; +args.mda_mode = perms; +res = make_dev_s(&args, &dev, name); +.Ed +.Pp +Similarly, the +.Fn make_dev_credf +function call is equivalent to +.Bd -literal -offset indent + (void) make_dev_s(&args, &dev, name); +.Ed +In other words, +.Fn make_dev_credf +does not allow the caller to obtain the return value, and in +kernels compiled with the +.Va INVARIANTS +options, the function asserts that the device creation succeeded. +.Pp +The .Fn make_dev_cred function is equivalent to the call .Bd -literal -offset indent @@ -207,45 +278,55 @@ make_dev_credf(0, cdevsw, unit, NULL, ui .Ed .Pp The -.Fn make_dev_p -function is similar to -.Fn make_dev_credf -but it may return an error number and takes a pointer to the resulting -.Ft *cdev -as an argument. -.Pp -The -.Fn make_dev_alias +.Fn make_dev_alias_p function takes the returned .Ft cdev from .Fn make_dev and makes another (aliased) name for this device. It is an error to call -.Fn make_dev_alias +.Fn make_dev_alias_p prior to calling .Fn make_dev . .Pp -.Fn make_dev_alias_p +The +.Fn make_dev_alias function is similar to .Fn make_dev_alias -but it takes a pointer to the resulting +but it returns the resulting aliasing .Ft *cdev -as an argument and may return an error. +and may not return an error. .Pp The .Fa cdev returned by -.Fn make_dev +.Fn make_dev_s and -.Fn make_dev_alias +.Fn make_dev_alias_p has two fields, .Fa si_drv1 and .Fa si_drv2 , that are available to store state. Both fields are of type -.Ft void * . +.Ft void * , +and can be initialized simultaneously with the +.Va cdev +allocation by filling +.Va args.mda_si_drv1 +and +.Va args.mda_si_drv2 +members of the +.Fn make_dev_s +argument structure, or filled after the +.Va cdev +is allocated, if using legacy interfaces. +In the latter case, the driver should handle the race of +accessing uninitialized +.Va si_drv1 +and +.Va si_drv2 +itself. These are designed to replace the .Fa unit argument to @@ -331,8 +412,10 @@ unload until is actually finished for all of them. .Sh RETURN VALUES If successful, +.Fn make_dev_s +and .Fn make_dev_p -will return 0, otherwise it will return an error. +will return 0, otherwise they will return an error. If successful, .Fn make_dev_credf will return a valid @@ -341,10 +424,11 @@ pointer, otherwise it will return .Dv NULL . .Sh ERRORS The +.Fn make_dev_s , .Fn make_dev_p and .Fn make_dev_alias_p -call will fail and the device will be not registered if: +calls will fail and the device will be not registered if: .Bl -tag -width Er .It Bq Er ENOMEM The @@ -403,3 +487,7 @@ The function .Fn make_dev_p first appeared in .Fx 8.2 . +The function +.Fn make_dev_s +first appeared in +.Fx 11.0 . Modified: head/sys/kern/kern_conf.c ============================================================================== --- head/sys/kern/kern_conf.c Thu Jan 7 19:58:23 2016 (r293345) +++ head/sys/kern/kern_conf.c Thu Jan 7 20:08:02 2016 (r293346) @@ -566,22 +566,26 @@ notify_destroy(struct cdev *dev) } static struct cdev * -newdev(struct cdevsw *csw, int unit, struct cdev *si) +newdev(struct make_dev_args *args, struct cdev *si) { struct cdev *si2; + struct cdevsw *csw; mtx_assert(&devmtx, MA_OWNED); + csw = args->mda_devsw; if (csw->d_flags & D_NEEDMINOR) { /* We may want to return an existing device */ LIST_FOREACH(si2, &csw->d_devs, si_list) { - if (dev2unit(si2) == unit) { + if (dev2unit(si2) == args->mda_unit) { dev_free_devlocked(si); return (si2); } } } - si->si_drv0 = unit; + si->si_drv0 = args->mda_unit; si->si_devsw = csw; + si->si_drv1 = args->mda_si_drv1; + si->si_drv2 = args->mda_si_drv2; LIST_INSERT_HEAD(&csw->d_devs, si, si_list); return (si); } @@ -737,33 +741,46 @@ prep_devname(struct cdev *dev, const cha return (0); } +void +make_dev_args_init_impl(struct make_dev_args *args, size_t sz) +{ + + bzero(args, sz); + args->mda_size = sz; +} + static int -make_dev_credv(int flags, struct cdev **dres, struct cdevsw *devsw, int unit, - struct ucred *cr, uid_t uid, gid_t gid, int mode, const char *fmt, - va_list ap) +make_dev_sv(struct make_dev_args *args1, struct cdev **dres, + const char *fmt, va_list ap) { struct cdev *dev, *dev_new; + struct make_dev_args args; int res; - KASSERT((flags & MAKEDEV_WAITOK) == 0 || (flags & MAKEDEV_NOWAIT) == 0, - ("make_dev_credv: both WAITOK and NOWAIT specified")); - dev_new = devfs_alloc(flags); + bzero(&args, sizeof(args)); + if (sizeof(args) < args1->mda_size) + return (EINVAL); + bcopy(args1, &args, args1->mda_size); + KASSERT((args.mda_flags & MAKEDEV_WAITOK) == 0 || + (args.mda_flags & MAKEDEV_NOWAIT) == 0, + ("make_dev_sv: both WAITOK and NOWAIT specified")); + dev_new = devfs_alloc(args.mda_flags); if (dev_new == NULL) return (ENOMEM); dev_lock(); - res = prep_cdevsw(devsw, flags); + res = prep_cdevsw(args.mda_devsw, args.mda_flags); if (res != 0) { dev_unlock(); devfs_free(dev_new); return (res); } - dev = newdev(devsw, unit, dev_new); + dev = newdev(&args, dev_new); if ((dev->si_flags & SI_NAMED) == 0) { res = prep_devname(dev, fmt, ap); if (res != 0) { - if ((flags & MAKEDEV_CHECKNAME) == 0) { + if ((args.mda_flags & MAKEDEV_CHECKNAME) == 0) { panic( - "make_dev_credv: bad si_name (error=%d, si_name=%s)", + "make_dev_sv: bad si_name (error=%d, si_name=%s)", res, dev->si_name); } if (dev == dev_new) { @@ -775,9 +792,9 @@ make_dev_credv(int flags, struct cdev ** return (res); } } - if (flags & MAKEDEV_REF) + if ((args.mda_flags & MAKEDEV_REF) != 0) dev_refl(dev); - if (flags & MAKEDEV_ETERNAL) + if ((args.mda_flags & MAKEDEV_ETERNAL) != 0) dev->si_flags |= SI_ETERNAL; if (dev->si_flags & SI_CHEAPCLONE && dev->si_flags & SI_NAMED) { @@ -792,24 +809,55 @@ make_dev_credv(int flags, struct cdev ** } KASSERT(!(dev->si_flags & SI_NAMED), ("make_dev() by driver %s on pre-existing device (min=%x, name=%s)", - devsw->d_name, dev2unit(dev), devtoname(dev))); + args.mda_devsw->d_name, dev2unit(dev), devtoname(dev))); dev->si_flags |= SI_NAMED; - if (cr != NULL) - dev->si_cred = crhold(cr); - dev->si_uid = uid; - dev->si_gid = gid; - dev->si_mode = mode; + if (args.mda_cr != NULL) + dev->si_cred = crhold(args.mda_cr); + dev->si_uid = args.mda_uid; + dev->si_gid = args.mda_gid; + dev->si_mode = args.mda_mode; devfs_create(dev); clean_unrhdrl(devfs_inos); dev_unlock_and_free(); - notify_create(dev, flags); + notify_create(dev, args.mda_flags); *dres = dev; return (0); } +int +make_dev_s(struct make_dev_args *args, struct cdev **dres, + const char *fmt, ...) +{ + va_list ap; + int res; + + va_start(ap, fmt); + res = make_dev_sv(args, dres, fmt, ap); + va_end(ap); + return (res); +} + +static int +make_dev_credv(int flags, struct cdev **dres, struct cdevsw *devsw, int unit, + struct ucred *cr, uid_t uid, gid_t gid, int mode, const char *fmt, + va_list ap) +{ + struct make_dev_args args; + + make_dev_args_init(&args); + args.mda_flags = flags; + args.mda_devsw = devsw; + args.mda_cr = cr; + args.mda_uid = uid; + args.mda_gid = gid; + args.mda_mode = mode; + args.mda_unit = unit; + return (make_dev_sv(&args, dres, fmt, ap)); +} + struct cdev * make_dev(struct cdevsw *devsw, int unit, uid_t uid, gid_t gid, int mode, const char *fmt, ...) @@ -1247,6 +1295,7 @@ clone_create(struct clonedevs **cdp, str { struct clonedevs *cd; struct cdev *dev, *ndev, *dl, *de; + struct make_dev_args args; int unit, low, u; KASSERT(*cdp != NULL, @@ -1298,7 +1347,10 @@ clone_create(struct clonedevs **cdp, str } if (unit == -1) unit = low & CLONE_UNITMASK; - dev = newdev(csw, unit | extra, ndev); + make_dev_args_init(&args); + args.mda_unit = unit | extra; + args.mda_devsw = csw; + dev = newdev(&args, ndev); if (dev->si_flags & SI_CLONELIST) { printf("dev %p (%s) is on clonelist\n", dev, dev->si_name); printf("unit=%d, low=%d, extra=0x%x\n", unit, low, extra); Modified: head/sys/sys/conf.h ============================================================================== --- head/sys/sys/conf.h Thu Jan 7 19:58:23 2016 (r293345) +++ head/sys/sys/conf.h Thu Jan 7 20:08:02 2016 (r293346) @@ -226,6 +226,28 @@ void clone_cleanup(struct clonedevs **); #define CLONE_FLAG0 (CLONE_UNITMASK + 1) int clone_create(struct clonedevs **, struct cdevsw *, int *unit, struct cdev **dev, int extra); +#define MAKEDEV_REF 0x01 +#define MAKEDEV_WHTOUT 0x02 +#define MAKEDEV_NOWAIT 0x04 +#define MAKEDEV_WAITOK 0x08 +#define MAKEDEV_ETERNAL 0x10 +#define MAKEDEV_CHECKNAME 0x20 +struct make_dev_args { + size_t mda_size; + int mda_flags; + struct cdevsw *mda_devsw; + struct ucred *mda_cr; + uid_t mda_uid; + gid_t mda_gid; + int mda_mode; + int mda_unit; + void *mda_si_drv1; + void *mda_si_drv2; +}; +void make_dev_args_init_impl(struct make_dev_args *_args, size_t _sz); +#define make_dev_args_init(a) \ + make_dev_args_init_impl((a), sizeof(struct make_dev_args)) + int count_dev(struct cdev *_dev); void delist_dev(struct cdev *_dev); void destroy_dev(struct cdev *_dev); @@ -245,12 +267,6 @@ struct cdev *make_dev(struct cdevsw *_de struct cdev *make_dev_cred(struct cdevsw *_devsw, int _unit, struct ucred *_cr, uid_t _uid, gid_t _gid, int _perms, const char *_fmt, ...) __printflike(7, 8); -#define MAKEDEV_REF 0x01 -#define MAKEDEV_WHTOUT 0x02 -#define MAKEDEV_NOWAIT 0x04 -#define MAKEDEV_WAITOK 0x08 -#define MAKEDEV_ETERNAL 0x10 -#define MAKEDEV_CHECKNAME 0x20 struct cdev *make_dev_credf(int _flags, struct cdevsw *_devsw, int _unit, struct ucred *_cr, uid_t _uid, gid_t _gid, int _mode, @@ -258,6 +274,8 @@ struct cdev *make_dev_credf(int _flags, int make_dev_p(int _flags, struct cdev **_cdev, struct cdevsw *_devsw, struct ucred *_cr, uid_t _uid, gid_t _gid, int _mode, const char *_fmt, ...) __printflike(8, 9); +int make_dev_s(struct make_dev_args *_args, struct cdev **_cdev, + const char *_fmt, ...) __printflike(3, 4); struct cdev *make_dev_alias(struct cdev *_pdev, const char *_fmt, ...) __printflike(2, 3); int make_dev_alias_p(int _flags, struct cdev **_cdev, struct cdev *_pdev, _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "[email protected]"
