| Message ID | 20240611093441.200910-1-carsten.haitzler@foss.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix alignment of temporary stack ioctl buffers | expand |
On Tue, Jun 11, 2024 at 2:35 AM <carsten.haitzler@foss.arm.com> wrote: > > From: Carsten Haitzler <carsten.haitzler@foss.arm.com> > > In a few places (core drm + AMD kfd driver), the ioctl handling uses a > temporary 128 byte buffer on the stack to copy to/from user. ioctl data > can have structs with types of much larger sizes than a byte and a > system may require alignment of types in these. At the same time the > compiler may align a char buf to something else as it has no idea that > this buffer is used for storing structs with such alignment > requirements. At a minimum putting in alignment information as an > attribute should be a warning in future if an architecture that needs > more alignment appears. > > This was discovered while implementing capability ABI support in Linux > on ARM's Morello CPU (128 bit capability "pointers" in userspace, with > a 64bit non-capability kernel (hybrid) setup). In this, userspace > ioctl structs now had to transport capabilities that needed 16 byte > alignment, but the kernel was not putting these data buffers on that > alignment boundary. > > Currently the largest type that is needed is a u64 so the alignment > only asks for that. Makes sense to me. Now that the kernel depends on C11, I think: __attribute__((aligned(__alignof__(u64)))) can be simply reduced to: _Alignas(u64) and put first instead of last in the declaration: _Alignas(u64) char stack_kdata[128]; > > Signed-off-by: Carsten Haitzler <carsten.haitzler@foss.arm.com> > --- > drivers/dma-buf/dma-heap.c | 2 +- > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- > drivers/gpu/drm/drm_ioctl.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 84ae708fafe7..a49e20440edf 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = { > static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > unsigned long arg) > { > - char stack_kdata[128]; > + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); > char *kdata = stack_kdata; > unsigned int kcmd; > unsigned int in_size, out_size, drv_size, ksize; > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > index fdf171ad4a3c..69a0aae0f016 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > @@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > amdkfd_ioctl_t *func; > const struct amdkfd_ioctl_desc *ioctl = NULL; > unsigned int nr = _IOC_NR(cmd); > - char stack_kdata[128]; > + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); > char *kdata = NULL; > unsigned int usize, asize; > int retcode = -EINVAL; > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index e368fc084c77..aac3d5a539a6 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp, > drm_ioctl_t *func; > unsigned int nr = DRM_IOCTL_NR(cmd); > int retcode = -EINVAL; > - char stack_kdata[128]; > + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); > char *kdata = NULL; > unsigned int in_size, out_size, drv_size, ksize; > bool is_driver_ioctl; > -- > 2.25.1 >
On 6/11/24 5:17 PM, T.J. Mercier wrote: > On Tue, Jun 11, 2024 at 2:35 AM <carsten.haitzler@foss.arm.com> wrote: >> >> From: Carsten Haitzler <carsten.haitzler@foss.arm.com> >> >> In a few places (core drm + AMD kfd driver), the ioctl handling uses a >> temporary 128 byte buffer on the stack to copy to/from user. ioctl data >> can have structs with types of much larger sizes than a byte and a >> system may require alignment of types in these. At the same time the >> compiler may align a char buf to something else as it has no idea that >> this buffer is used for storing structs with such alignment >> requirements. At a minimum putting in alignment information as an >> attribute should be a warning in future if an architecture that needs >> more alignment appears. >> >> This was discovered while implementing capability ABI support in Linux >> on ARM's Morello CPU (128 bit capability "pointers" in userspace, with >> a 64bit non-capability kernel (hybrid) setup). In this, userspace >> ioctl structs now had to transport capabilities that needed 16 byte >> alignment, but the kernel was not putting these data buffers on that >> alignment boundary. >> >> Currently the largest type that is needed is a u64 so the alignment >> only asks for that. > > Makes sense to me. > > Now that the kernel depends on C11, I think: > __attribute__((aligned(__alignof__(u64)))) Aaaah yes. I'm living in the past as usual. :) > can be simply reduced to: > _Alignas(u64) > > and put first instead of last in the declaration: > _Alignas(u64) char stack_kdata[128]; Yup. Will do. Expect a V2 to come in. >> >> Signed-off-by: Carsten Haitzler <carsten.haitzler@foss.arm.com> >> --- >> drivers/dma-buf/dma-heap.c | 2 +- >> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- >> drivers/gpu/drm/drm_ioctl.c | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c >> index 84ae708fafe7..a49e20440edf 100644 >> --- a/drivers/dma-buf/dma-heap.c >> +++ b/drivers/dma-buf/dma-heap.c >> @@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = { >> static long dma_heap_ioctl(struct file *file, unsigned int ucmd, >> unsigned long arg) >> { >> - char stack_kdata[128]; >> + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); >> char *kdata = stack_kdata; >> unsigned int kcmd; >> unsigned int in_size, out_size, drv_size, ksize; >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> index fdf171ad4a3c..69a0aae0f016 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> @@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) >> amdkfd_ioctl_t *func; >> const struct amdkfd_ioctl_desc *ioctl = NULL; >> unsigned int nr = _IOC_NR(cmd); >> - char stack_kdata[128]; >> + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); >> char *kdata = NULL; >> unsigned int usize, asize; >> int retcode = -EINVAL; >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index e368fc084c77..aac3d5a539a6 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp, >> drm_ioctl_t *func; >> unsigned int nr = DRM_IOCTL_NR(cmd); >> int retcode = -EINVAL; >> - char stack_kdata[128]; >> + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); >> char *kdata = NULL; >> unsigned int in_size, out_size, drv_size, ksize; >> bool is_driver_ioctl; >> -- >> 2.25.1 >>
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 84ae708fafe7..a49e20440edf 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = { static long dma_heap_ioctl(struct file *file, unsigned int ucmd, unsigned long arg) { - char stack_kdata[128]; + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); char *kdata = stack_kdata; unsigned int kcmd; unsigned int in_size, out_size, drv_size, ksize; diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index fdf171ad4a3c..69a0aae0f016 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) amdkfd_ioctl_t *func; const struct amdkfd_ioctl_desc *ioctl = NULL; unsigned int nr = _IOC_NR(cmd); - char stack_kdata[128]; + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); char *kdata = NULL; unsigned int usize, asize; int retcode = -EINVAL; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index e368fc084c77..aac3d5a539a6 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp, drm_ioctl_t *func; unsigned int nr = DRM_IOCTL_NR(cmd); int retcode = -EINVAL; - char stack_kdata[128]; + char stack_kdata[128] __attribute__((aligned(__alignof__(u64)))); char *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl;