svn commit: r321920 - head/sys/sys

[Konstantin Belousov]

On Wed, Aug 02, 2017 at 12:35:47PM +0200, Hans Petter Selasky wrote:
> On 08/02/17 12:14, Konstantin Belousov wrote:
> > Author: kib
> > Date: Wed Aug  2 10:14:17 2017
> > New Revision: 321920
> > URL: https://svnweb.freebsd.org/changeset/base/321920
> > 
> > Log:
> >    Change major()/minor() to work with 64bit dev_t.
> >    
> >    Since traditional types for the macros values are int, remove the
> >    cookie trick and just split the dev_t at the word boundary.
> >    
> >    Reported by:	Victor Stinner <victor.stinner at gmail.com>
> >    PR:	221048
> >    Sponsored by:	The FreeBSD Foundation
> > 
> > Modified:
> >    head/sys/sys/types.h
> > 
> > Modified: head/sys/sys/types.h
> > ==============================================================================
> > --- head/sys/sys/types.h	Wed Aug  2 10:12:10 2017	(r321919)
> > +++ head/sys/sys/types.h	Wed Aug  2 10:14:17 2017	(r321920)
> > @@ -364,14 +364,9 @@ __bitcount64(__uint64_t _x)
> >   
> >   #include <sys/select.h>
> >   
> > -/*
> > - * minor() gives a cookie instead of an index since we don't want to
> > - * change the meanings of bits 0-15 or waste time and space shifting
> > - * bits 16-31 for devices that don't use them.
> > - */
> > -#define	major(x)	((int)(((u_int)(x) >> 8)&0xff))	/* major number */
> > -#define	minor(x)	((int)((x)&0xffff00ff))		/* minor number */
> > -#define	makedev(x,y)	((dev_t)(((x) << 8) | (y)))	/* create dev_t */
> > +#define	major(x)	((int)((dev_t)(x) >> 32))	/* major number */
> > +#define	minor(x)	((int)((x) & 0xffffffff))	/* minor number */
> > +#define	makedev(x, y)	(((dev_t)(x) << 32) | (y))	/* create dev_t */
> >   
> >   /*
> >    * These declarations belong elsewhere, but are repeated here and in
> > 
> 
> Hi,
> 
> This change breaks the LinuxKPI:
> 
> > struct linux_cdev *
> > linux_find_cdev(const char *name, unsigned major, unsigned minor)
> > {
> >         int unit = MKDEV(major, minor);
> >         struct cdev *cdev;
> >   
> >         dev_lock();
> >         LIST_FOREACH(cdev, &linuxcdevsw.d_devs, si_list) {
> >                 struct linux_cdev *ldev = cdev->si_drv1;
> >                 if (dev2unit(cdev) == unit &&
> >                     strcmp(kobject_name(&ldev->kobj), name) == 0) {
> >                         break;
> >                 }
> >         }
> >         dev_unlock();
> >  
> >         return (cdev != NULL ? cdev->si_drv1 : NULL);
> > }
So linuxkpi was broken before this commit as well, as I noted in my
other reply ?

> 
> You will also need to change cdev->si_drv0 to dev_t  and fix the 
> make_dev() functions too.
Why do you think that si_drv0 needs to be changes ?

> 
> > struct cdev {
> >         void            *si_spare0;
> >         u_int           si_flags;
> > #define SI_ETERNAL      0x0001  /* never destroyed */
> > #define SI_ALIAS        0x0002  /* carrier of alias name */
> > #define SI_NAMED        0x0004  /* make_dev{_alias} has been called */
> > #define SI_CHEAPCLONE   0x0008  /* can be removed_dev'ed when vnode reclaims */
> > #define SI_CHILD        0x0010  /* child of another struct cdev **/
> > #define SI_DUMPDEV      0x0080  /* is kernel dumpdev */
> > #define SI_CLONELIST    0x0200  /* on a clone list */
> > #define SI_UNMAPPED     0x0400  /* can handle unmapped I/O */
> > #define SI_NOSPLIT      0x0800  /* I/O should not be split up */
> >         struct timespec si_atime;
> >         struct timespec si_ctime;
> >         struct timespec si_mtime;
> >         uid_t           si_uid;
> >         gid_t           si_gid;
> >         mode_t          si_mode;
> >         struct ucred    *si_cred;       /* cached clone-time credential */
> >         int             si_drv0;
> 
> 
> Further this update assigns a 64-bit value to tfsid.val[0], which is 
> 32-bit ??
> 
>  > sys/sys/mount.h:typedef struct fsid { int32_t val[2]; } fsid_t;	/* 
> filesystem id type */
> 
> sys/kern/vfs_subr.c:            tfsid.val[0] = makedev(255,
>From my reading if the vfs_getnewfsid() code, it is fine.  It ignores
the 255-major value, which does not add much sense to the returned value,
just that byte 1 is not zero and not 0xff.

> 
> And there are more places like this!
I see the use of makedev() in nfsserver, where the change seemingly improve
the situation by initializing na_rdev to the full 64 bit value, and in
fdescfs, which does not care.

> 
> Did you properly check all uses of makedev() in the kernel before making 
> this change?
> 
> --HPS