On Wed, May 11, 2022 at 09:49:34AM -0600, Theo de Raadt wrote:
> Alexander Bluhm <[email protected]> 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
