On Wed, May 11, 2022 at 09:49:34AM -0600, Theo de Raadt wrote: > Alexander Bluhm <alexander.bl...@gmx.net> wrote: > > > On Wed, May 11, 2022 at 11:20:15AM +0300, Vitaliy Makkoveev wrote: > > > sys_umask() only modifies `fd_cmask', which modification is already > > > protected by `fd_lock' rwlock(9). > > > > I found this in sys/filedesc.h > > > > u_short fd_cmask; /* [f/w] mask for file creation */ > > u_short fd_refcnt; /* [K] reference count */ > > > > We have two short variables that are protected by different locks. > > I think 16 bit values are not MP independent on all architectures. > > > > When one CPU modifies the lower 16 bit and another CPU writes to > > the higher 16 bit the result in the full 32 bit is not defined. > > This is at least my understanding. > > Such as the oldest alpha cpus, before the BWX extensions were added. > > > I have seen problems in real live with two shorts when one 16 bit > > part was changed without spl protection and the other 16 bits were > > written by interrupt. > > > > Should we convert them to u_int? > > I think fd_cmask should be changed to the proper type: mode_t, which is > __mode_t, which is __uint32_t > > Once that expands to a 32-bit value, there is no need to try to be > space-optimal with fd_refcnt, it may as well be an int. > > Funny thing. On 64-bit architectures the alignment and padding rules > are placing these fields at these offsets: > > fd_cmask 64 > fd_refcnt 66 > fd_lock 72 > > So 68-71 are 4-pad bytes to align fd_lock. Changing both types to int > will not grow this structure on 64-bit architectures. >
Updated diff. I changed `fd_cmask' to mode_t. The type change of `fd_refcnt' is not related to umask(2) unlocking, so I want to made it with separate diff. Index: sys/kern/syscalls.master =================================================================== RCS file: /cvs/src/sys/kern/syscalls.master,v retrieving revision 1.223 diff -u -p -r1.223 syscalls.master --- sys/kern/syscalls.master 24 Feb 2022 07:41:51 -0000 1.223 +++ sys/kern/syscalls.master 11 May 2022 19:05:12 -0000 @@ -146,7 +146,7 @@ char *buf, size_t count); } 59 STD { int sys_execve(const char *path, \ char * const *argp, char * const *envp); } -60 STD { mode_t sys_umask(mode_t newmask); } +60 STD NOLOCK { mode_t sys_umask(mode_t newmask); } 61 STD { int sys_chroot(const char *path); } 62 STD { int sys_getfsstat(struct statfs *buf, size_t bufsize, \ int flags); } Index: sys/sys/filedesc.h =================================================================== RCS file: /cvs/src/sys/sys/filedesc.h,v retrieving revision 1.45 diff -u -p -r1.45 filedesc.h --- sys/sys/filedesc.h 4 Jul 2020 08:06:08 -0000 1.45 +++ sys/sys/filedesc.h 11 May 2022 19:05:12 -0000 @@ -79,7 +79,7 @@ struct filedesc { u_int *fd_lomap; /* [f] bitmap of free fds */ int fd_lastfile; /* [f] high-water mark of fd_ofiles */ int fd_freefile; /* [f] approx. next free file */ - u_short fd_cmask; /* [f/w] mask for file creation */ + mode_t fd_cmask; /* [f/w] mask for file creation */ u_short fd_refcnt; /* [K] reference count */ struct rwlock fd_lock; /* lock for the file descs */ struct mutex fd_fplock; /* lock for reading fd_ofiles without