[PATCH] fs: don't randomized kiocb fields

[PATCH] fs: don't randomized kiocb fields

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This is a size sensitive structure and randomizing can introduce extra
padding that breaks io_uring's fixed size expectations. There are few
fields here as it is, half of which need a fixed order to optimally
pack, so the randomization isn't providing much.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/fs.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5113f65c786f..9eced4cc286e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -340,17 +340,12 @@ enum rw_hint {
 
 struct kiocb {
 	struct file		*ki_filp;
-
-	/* The 'ki_filp' pointer is shared in a union for aio */
-	randomized_struct_fields_start
-
 	loff_t			ki_pos;
 	void (*ki_complete)(struct kiocb *iocb, long ret);
 	void			*private;
 	int			ki_flags;
 	u16			ki_ioprio; /* See linux/ioprio.h */
 	struct wait_page_queue	*ki_waitq; /* for async buffered IO */
-	randomized_struct_fields_end
 };
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
-- 
2.30.2

Re: [PATCH] fs: don't randomized kiocb fields

[Jens Axboe]

On 8/12/22 4:56 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This is a size sensitive structure and randomizing can introduce extra
> padding that breaks io_uring's fixed size expectations. There are few
> fields here as it is, half of which need a fixed order to optimally
> pack, so the randomization isn't providing much.

I'll add the link to the previous discussion as well in the commit:

Link: https://lore.kernel.org/io-uring/b6f508ca-b1b2-5f40-7998-e4cff1cf7212@kernel.dk/

as it's pretty useful for context.

-- 
Jens Axboe

Re: [PATCH] fs: don't randomized kiocb fields

[Christoph Hellwig]

s/randomized/randomize/

in the subject?

Re: [PATCH] fs: don't randomized kiocb fields

[Jens Axboe]

On 8/13/22 1:59 AM, Christoph Hellwig wrote:
> s/randomized/randomize/
> 
> in the subject?

I did notice that one and made the edit yesterday.

-- 
Jens Axboe

Re: [PATCH] fs: don't randomized kiocb fields

[Matthew Wilcox]

On Fri, Aug 12, 2022 at 03:56:33PM -0700, Keith Busch wrote:
>  struct kiocb {
>  	struct file		*ki_filp;
> -
> -	/* The 'ki_filp' pointer is shared in a union for aio */
> -	randomized_struct_fields_start
> -
>  	loff_t			ki_pos;
>  	void (*ki_complete)(struct kiocb *iocb, long ret);
>  	void			*private;
>  	int			ki_flags;
>  	u16			ki_ioprio; /* See linux/ioprio.h */
>  	struct wait_page_queue	*ki_waitq; /* for async buffered IO */
> -	randomized_struct_fields_end
>  };

Now that I've read the thread ...

If we care about struct size on 32-bit, we should fit something into
the 32-bit hole before the 64-bit loff_t (assuming at least some 32-bit
arches want loff_t to be 64-bit aligned; I thik x86 doesn't?)
Easiest seems to be to put ki_complete before ki_pos?

Re: [PATCH] fs: don't randomized kiocb fields

[Keith Busch]

On Sun, Aug 14, 2022 at 11:35:35AM +0100, Matthew Wilcox wrote:
> On Fri, Aug 12, 2022 at 03:56:33PM -0700, Keith Busch wrote:
> >  struct kiocb {
> >  	struct file		*ki_filp;
> > -
> > -	/* The 'ki_filp' pointer is shared in a union for aio */
> > -	randomized_struct_fields_start
> > -
> >  	loff_t			ki_pos;
> >  	void (*ki_complete)(struct kiocb *iocb, long ret);
> >  	void			*private;
> >  	int			ki_flags;
> >  	u16			ki_ioprio; /* See linux/ioprio.h */
> >  	struct wait_page_queue	*ki_waitq; /* for async buffered IO */
> > -	randomized_struct_fields_end
> >  };
> 
> Now that I've read the thread ...
> 
> If we care about struct size on 32-bit, we should fit something into
> the 32-bit hole before the 64-bit loff_t (assuming at least some 32-bit
> arches want loff_t to be 64-bit aligned; I thik x86 doesn't?)
> Easiest seems to be to put ki_complete before ki_pos?

Yes, that would be a better compact arrangment. The immediate need was just to
correct io_uring's max command struct size. The 32-bit verison isn't near the
limit, so no additional optimizations were done in this patch.