| Message ID | 153616291029.23468.16421004714304578585.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] UAPI: drm: Fix use of C++ keywords as structural members | expand |
On Wed, Sep 05, 2018 at 04:55:10PM +0100, David Howells wrote: > The size and layout of internal kernel structures may not be relied upon > outside of the kernel and may even change in a containerised environment if > a container image is frozen and shifted to another machine. > > Excise these from Coda's upc_req struct. Argh, that won't work. I still have to look at where this structure is used exactly, but... Either this structure is used by the messages that the kernel sends to userspace, in which case we don't want the kernel to pack the larger structure that includes a list_head and a wait_queue_head_t in the message while userspace reads as if it was a smaller structure without those. But my gut feeling is that this is not part of the upcall request messages and never gets to userspace and as such shouldn't be in uapi to begin with. Jan
Le mercredi 05 septembre 2018 à 16:55 +0100, David Howells a écrit : > The size and layout of internal kernel structures may not be relied > upon outside of the kernel and may even change in a containerised > environment if a container image is frozen and shifted to another > machine. > > Excise these from Coda's upc_req struct. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Jan Harkes <jaharkes@cs.cmu.edu> > cc: coda@cs.cmu.edu > cc: codalist@coda.cs.cmu.edu > cc: linux-fsdevel@vger.kernel.org > --- > > include/uapi/linux/coda_psdev.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/uapi/linux/coda_psdev.h b/include/uapi/linux/coda_psdev.h > index aa6623efd2dd..9c3acde393cd 100644 > --- a/include/uapi/linux/coda_psdev.h > +++ b/include/uapi/linux/coda_psdev.h > @@ -10,14 +10,18 @@ > > /* messages between coda filesystem in kernel and Venus */ > struct upc_req { > +#ifdef __KERNEL__ > struct list_head uc_chain; > +#endif > caddr_t uc_data; > u_short uc_flags; > u_short uc_inSize; /* Size is at most 5000 bytes */ > u_short uc_outSize; > u_short uc_opcode; /* copied from data to save lookup */ > int uc_unique; > +#ifdef __KERNEL__ > wait_queue_head_t uc_sleep; /* process' wait queue */ > +#endif > }; > This structure should not have been exposed to userspace in the first place: it's unusable by userspace as it is. It was incorrect to have it outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... ... and it's not exchanged between kernel and userspace, see coda_psdev_write(): struct upc_req *req = NULL; ... if (copy_from_user(req->uc_data, buf, nbytes)) { req->uc_flags |= CODA_REQ_ABORT; wake_up(&req->uc_sleep); retval = -EFAULT; goto out; } Only data, a caddr_t, is read from userspace. So the structure can be moved back to <linux/coda_psdev.h>. > #define CODA_REQ_ASYNC 0x1 > All CODA_REQ_* defines internals to kernel side and not exchanged with userspace. Please move them back to <linux/coda_psdev.h> Regards.
Yann Droneaud <ydroneaud@opteya.com> wrote:
> Please move them back to <linux/coda_psdev.h>
Will do.
David
On Wed, Sep 05, 2018 at 07:12:37PM +0200, Yann Droneaud wrote: > Le mercredi 05 septembre 2018 à 16:55 +0100, David Howells a écrit : > > The size and layout of internal kernel structures may not be relied > > upon outside of the kernel and may even change in a containerised > > environment if a container image is frozen and shifted to another > > machine. > > > > Excise these from Coda's upc_req struct. ... > > This structure should not have been exposed to userspace in the first > place: it's unusable by userspace as it is. It was incorrect to have it > outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... ... > So the structure can be moved back to <linux/coda_psdev.h>. I found a year old patch that clearly fell through the cracks that fixes this exact thing. https://lkml.org/lkml/2017/8/6/186 Jan
Yann Droneaud <ydroneaud@opteya.com> wrote: > This structure should not have been exposed to userspace in the first > place: it's unusable by userspace as it is. It was incorrect to have it > outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... > ... > All CODA_REQ_* defines internals to kernel side and not exchanged with > userspace. > > Please move them back to <linux/coda_psdev.h> Is there any reason coda_psdev.h needs to be in include/linux/ rather than fs/coda/? David
Hi, Le jeudi 06 septembre 2018 à 08:13 +0100, David Howells a écrit : > Yann Droneaud <ydroneaud@opteya.com> wrote: > > > This structure should not have been exposed to userspace in the > > first > > place: it's unusable by userspace as it is. It was incorrect to > > have it > > outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... > > ... > > All CODA_REQ_* defines internals to kernel side and not exchanged > > with > > userspace. > > > > Please move them back to <linux/coda_psdev.h> > > Is there any reason coda_psdev.h needs to be in include/linux/ rather > than fs/coda/? > It's a valid concern. At first I thought the first lines (see below) could have been useful for userspace: #define CODA_PSDEV_MAJOR 67 #define MAX_CODADEVS 5 /* how many do we allow */ But the file was unsuable for a long long time so we can assume it's usage by userspace is deprecated, then we could remove it from UAPI, and moves its content back to include/linux. As one could see include/linux/coda_psdev.h is not used outside of fs/coda, moving the header here as you suggests seems to be the correct solution. Regards.
On Thu, Sep 06, 2018 at 01:52:29PM +0200, Yann Droneaud wrote: > Hi, > > Le jeudi 06 septembre 2018 à 08:13 +0100, David Howells a écrit : > > Yann Droneaud <ydroneaud@opteya.com> wrote: > > > > > This structure should not have been exposed to userspace in the > > > first > > > place: it's unusable by userspace as it is. It was incorrect to > > > have it > > > outside of #ifdef __KERNEL__ before commit 607ca46e97a1b ... > > > ... > > > All CODA_REQ_* defines internals to kernel side and not exchanged > > > with > > > userspace. > > > > > > Please move them back to <linux/coda_psdev.h> > > > > Is there any reason coda_psdev.h needs to be in include/linux/ rather > > than fs/coda/? > > > > It's a valid concern. > > At first I thought the first lines (see below) could have been useful > for userspace: > > #define CODA_PSDEV_MAJOR 67 > #define MAX_CODADEVS 5 /* how many do we allow */ Nope, userspace just tries to open /dev/cfs0, or a manually configured alternative. We have only used linux/coda.h, and actually carry our own copy of that file which is kept in sync manually, which is why there are all those ifdefs for different systems in there. This all originates from the time of the 2.1.x kernels when Coda was built externally. > But the file was unsuable for a long long time so we can assume it's > usage by userspace is deprecated, then we could remove it from UAPI, > and moves its content back to include/linux. > > As one could see include/linux/coda_psdev.h is not used outside of > fs/coda, moving the header here as you suggests seems to be the correct > solution. Agreed. Jan
Yann Droneaud <ydroneaud@opteya.com> wrote: > At first I thought the first lines (see below) could have been useful > for userspace: > > #define CODA_PSDEV_MAJOR 67 > #define MAX_CODADEVS 5 /* how many do we allow */ Note that I was asking about include/linux/coda_psdev.h (the internal kernel header), not include/uapi/linux/coda_psdev.h (the UAPI header). David
diff --git a/include/uapi/linux/coda_psdev.h b/include/uapi/linux/coda_psdev.h index aa6623efd2dd..9c3acde393cd 100644 --- a/include/uapi/linux/coda_psdev.h +++ b/include/uapi/linux/coda_psdev.h @@ -10,14 +10,18 @@ /* messages between coda filesystem in kernel and Venus */ struct upc_req { +#ifdef __KERNEL__ struct list_head uc_chain; +#endif caddr_t uc_data; u_short uc_flags; u_short uc_inSize; /* Size is at most 5000 bytes */ u_short uc_outSize; u_short uc_opcode; /* copied from data to save lookup */ int uc_unique; +#ifdef __KERNEL__ wait_queue_head_t uc_sleep; /* process' wait queue */ +#endif }; #define CODA_REQ_ASYNC 0x1
The size and layout of internal kernel structures may not be relied upon outside of the kernel and may even change in a containerised environment if a container image is frozen and shifted to another machine. Excise these from Coda's upc_req struct. Signed-off-by: David Howells <dhowells@redhat.com> cc: Jan Harkes <jaharkes@cs.cmu.edu> cc: coda@cs.cmu.edu cc: codalist@coda.cs.cmu.edu cc: linux-fsdevel@vger.kernel.org --- include/uapi/linux/coda_psdev.h | 4 ++++ 1 file changed, 4 insertions(+)