Date: Sun, 5 Oct 2014 12:51:56 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Konstantin Belousov <kib@freebsd.org>, Mateusz Guzik <mjg@freebsd.org> Subject: Re: svn commit: r272505 - in head/sys: kern sys Message-ID: <20141005123549.A1162@besplex.bde.org> In-Reply-To: <42180557-0119-4597-9492-662E1671A840@FreeBSD.org> References: <201410040808.s9488uAI099166@svn.freebsd.org> <42180557-0119-4597-9492-662E1671A840@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 4 Oct 2014, Bjoern A. Zeeb wrote:
> On 04 Oct 2014, at 08:08 , Mateusz Guzik <mjg@FreeBSD.org> wrote:
>> ...
>> Log:
>> Plug capability races.
> ...
>
> This file is included from user space. There is no opt_capsicum.h there.
> Including an opt_* in the header file seems wrong in a lot of ways usually.
>
> I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse.
>
> This needs a better fix.
>
> I also wonder why the (conditional) fde_seq ended up at the beginning of the structure rather than the end?
It adds to the already-massive namespace pollution in other ways.
>> Modified: head/sys/sys/filedesc.h
>> ==============================================================================
>> --- head/sys/sys/filedesc.h Sat Oct 4 08:05:39 2014 (r272504)
>> +++ head/sys/sys/filedesc.h Sat Oct 4 08:08:56 2014 (r272505)
>> @@ -33,11 +33,14 @@
>> #ifndef _SYS_FILEDESC_H_
>> #define _SYS_FILEDESC_H_
>>
>> +#include "opt_capsicum.h"
>> +
Fatal namspace pollution.
>> #include <sys/caprights.h>
>> #include <sys/queue.h>
>> #include <sys/event.h>
>> #include <sys/lock.h>
>> #include <sys/priority.h>
Old massive namespace pollution.
>> +#include <sys/seq.h>
New namespace pollution.
>> #include <sys/sx.h>
>>
>> #include <machine/_limits.h>
Old massive namespace pollution, continued.
The pollution is nested. <sys/sx.h> at least has a _KERNEL ifdef around
most of its pollution.
>> @@ -50,6 +53,9 @@ struct filecaps {
>> };
>>
>> struct filedescent {
>> +#ifdef CAPABILITIES
>> + seq_t fde_seq; /* if you need fde_file and fde_caps in sync */
>> +#endif
This ifdef makes the struct not really a struct. The layout depends on
visible options, and the visible options may depend on pollution.
The worst sort of pollution bug would occur if something else has a
similar ifdef but doesn't include the options header. Then when different
pollution there results in including this header, the include of the
options header here gives different visible options and thus a different
struct layout there.
>> struct file *fde_file; /* file structure for open file */
>> struct filecaps fde_caps; /* per-descriptor rights */
>> uint8_t fde_flags; /* per-process open file flags */
>> @@ -58,6 +64,13 @@ struct filedescent {
>> #define fde_fcntls fde_caps.fc_fcntls
>> #define fde_ioctls fde_caps.fc_ioctls
>> #define fde_nioctls fde_caps.fc_nioctls
>> +#ifdef CAPABILITIES
>> +#define fde_change(fde) ((char *)(fde) + sizeof(seq_t))
>> +#define fde_change_size (sizeof(struct filedescent) - sizeof(seq_t))
>> +#else
>> +#define fde_change(fde) ((fde))
>> +#define fde_change_size (sizeof(struct filedescent))
>> +#endif
>>
>> /*
>> * This structure is used for the management of descriptors. It may be
>> @@ -82,6 +95,9 @@ struct filedesc {
>> int fd_holdleaderscount; /* block fdfree() for shared close() */
>> int fd_holdleaderswakeup; /* fdfree() needs wakeup */
>> };
>> +#ifdef CAPABILITIES
>> +#define fd_seq(fdp, fd) (&(fdp)->fd_ofiles[(fd)].fde_seq)
>> +#endif
The Ifdefs of the macros are not even wrong. Macros are not as
polluting as inline functions, so always defining them doen't cause
many problems. They just give relatively minor namespace pollution
and are unusable if the headers needed to use them are not included.
The patch has some style bugs, e.g., long lines.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141005123549.A1162>