[PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Denys Vlasenko]

Suggested by Linus.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 include/linux/stddef.h |  9 +++++++++
 include/linux/vfio.h   | 13 -------------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index f4aec0e..076af43 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -19,3 +19,12 @@ enum {
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
 #endif
+
+/**
+ * offsetofend(TYPE, MEMBER)
+ *
+ * @TYPE: The type of the structure
+ * @MEMBER: The member within the structure to get the end offset of
+ */
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 2d67b89..049b2f4 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -78,19 +78,6 @@ extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
 extern void vfio_unregister_iommu_driver(
 				const struct vfio_iommu_driver_ops *ops);
 
-/**
- * offsetofend(TYPE, MEMBER)
- *
- * @TYPE: The type of the structure
- * @MEMBER: The member within the structure to get the end offset of
- *
- * Simple helper macro for dealing with variable sized structures passed
- * from user space.  This allows us to easily determine if the provided
- * structure is sized to include various fields.
- */
-#define offsetofend(TYPE, MEMBER) \
-	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
-
 /*
  * External user API
  */
-- 
1.8.1.4

[PATCH 2/2 v2] x86: make 32-bit "emergency stack" better documented

[Denys Vlasenko]

Before the patch, tss.stack field was not referenced anywhere.
It was used only by setting sysenter's stack to point after
last byte of tss, thus the trailing field, stack[64], was used.

But grep would not know it. You can comment it out, compile,
and kernel will even run until an unlucky NMI corrupts
io_bitmap[] (which is also not easily detectable).

This patch changes code so that the purpose and usage of this field
is not mysterious anymore, and can be easily grepped for.

This does change generated code, for a subtle reason:
since tss_struct is ____cacheline_aligned, there happen to be
5 longs of padding at the end. Old code was using the padding too;
new code will strictly use only SYSENTER_stack[].

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
Changes since v1: use offsetofend()

 arch/x86/include/asm/processor.h | 4 ++--
 arch/x86/kernel/asm-offsets_32.c | 2 +-
 arch/x86/kernel/cpu/common.c     | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 48a61c1..9e65cf8 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -276,9 +276,9 @@ struct tss_struct {
 	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
 
 	/*
-	 * .. and then another 0x100 bytes for the emergency kernel stack:
+	 * and then space for temporary SYSENTER stack:
 	 */
-	unsigned long		stack[64];
+	unsigned long		SYSENTER_stack[64];
 
 } ____cacheline_aligned;
 
diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
index 3b3b9d3..42a3b28 100644
--- a/arch/x86/kernel/asm-offsets_32.c
+++ b/arch/x86/kernel/asm-offsets_32.c
@@ -68,7 +68,7 @@ void foo(void)
 
 	/* Offset from the sysenter stack to tss.sp0 */
 	DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
-		 sizeof(struct tss_struct));
+		offsetofend(struct tss_struct, SYSENTER_stack));
 
 #if defined(CONFIG_LGUEST) || defined(CONFIG_LGUEST_GUEST) || defined(CONFIG_LGUEST_MODULE)
 	BLANK();
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 7634833..4701293 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -987,7 +987,8 @@ void enable_sep_cpu(void)
 	}
 
 	tss->x86_tss.ss1 = __KERNEL_CS;
-	tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
+	tss->x86_tss.sp1 = (unsigned long) tss
+			+ offsetofend(struct tss_struct, SYSENTER_stack);
 	wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 	wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
 	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);
-- 
1.8.1.4

Re: [PATCH 2/2 v2] x86: make 32-bit "emergency stack" better documented

[Andy Lutomirski]

On Mon, Mar 9, 2015 at 7:52 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Before the patch, tss.stack field was not referenced anywhere.
> It was used only by setting sysenter's stack to point after
> last byte of tss, thus the trailing field, stack[64], was used.
>
> But grep would not know it. You can comment it out, compile,
> and kernel will even run until an unlucky NMI corrupts
> io_bitmap[] (which is also not easily detectable).
>
> This patch changes code so that the purpose and usage of this field
> is not mysterious anymore, and can be easily grepped for.
>
> This does change generated code, for a subtle reason:
> since tss_struct is ____cacheline_aligned, there happen to be
> 5 longs of padding at the end. Old code was using the padding too;
> new code will strictly use only SYSENTER_stack[].

Acked-by: Andy Lutomirski <luto@amacapital.net>

>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
> Changes since v1: use offsetofend()
>
>  arch/x86/include/asm/processor.h | 4 ++--
>  arch/x86/kernel/asm-offsets_32.c | 2 +-
>  arch/x86/kernel/cpu/common.c     | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 48a61c1..9e65cf8 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -276,9 +276,9 @@ struct tss_struct {
>         unsigned long           io_bitmap[IO_BITMAP_LONGS + 1];
>
>         /*
> -        * .. and then another 0x100 bytes for the emergency kernel stack:
> +        * and then space for temporary SYSENTER stack:
>          */
> -       unsigned long           stack[64];
> +       unsigned long           SYSENTER_stack[64];
>
>  } ____cacheline_aligned;
>
> diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
> index 3b3b9d3..42a3b28 100644
> --- a/arch/x86/kernel/asm-offsets_32.c
> +++ b/arch/x86/kernel/asm-offsets_32.c
> @@ -68,7 +68,7 @@ void foo(void)
>
>         /* Offset from the sysenter stack to tss.sp0 */
>         DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
> -                sizeof(struct tss_struct));
> +               offsetofend(struct tss_struct, SYSENTER_stack));
>
>  #if defined(CONFIG_LGUEST) || defined(CONFIG_LGUEST_GUEST) || defined(CONFIG_LGUEST_MODULE)
>         BLANK();
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 7634833..4701293 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -987,7 +987,8 @@ void enable_sep_cpu(void)
>         }
>
>         tss->x86_tss.ss1 = __KERNEL_CS;
> -       tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
> +       tss->x86_tss.sp1 = (unsigned long) tss
> +                       + offsetofend(struct tss_struct, SYSENTER_stack);
>         wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
>         wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
>         wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);
> --
> 1.8.1.4
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 2/2 v2] x86: make 32-bit "emergency stack" better documented

[Pavel Machek]

Hi!

>  arch/x86/kernel/asm-offsets_32.c | 2 +-
>  arch/x86/kernel/cpu/common.c     | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 48a61c1..9e65cf8 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -276,9 +276,9 @@ struct tss_struct {
>  	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
>  
>  	/*
> -	 * .. and then another 0x100 bytes for the emergency kernel stack:
> +	 * and then space for temporary SYSENTER stack:
>  	 */
> -	unsigned long		stack[64];
> +	unsigned long		SYSENTER_stack[64];

Would it make sense to make it unsigned char ...stack[256], like the comment said?

Or is it supposed to be different size on 64bit, and comment was wrong?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2 v2] x86: make 32-bit "emergency stack" better documented

[Brian Gerst]

On Sat, Mar 14, 2015 at 12:00 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>>  arch/x86/kernel/asm-offsets_32.c | 2 +-
>>  arch/x86/kernel/cpu/common.c     | 3 ++-
>>  3 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 48a61c1..9e65cf8 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -276,9 +276,9 @@ struct tss_struct {
>>       unsigned long           io_bitmap[IO_BITMAP_LONGS + 1];
>>
>>       /*
>> -      * .. and then another 0x100 bytes for the emergency kernel stack:
>> +      * and then space for temporary SYSENTER stack:
>>        */
>> -     unsigned long           stack[64];
>> +     unsigned long           SYSENTER_stack[64];
>
> Would it make sense to make it unsigned char ...stack[256], like the comment said?
>
> Or is it supposed to be different size on 64bit, and comment was wrong?

It isn't used on 64-bit.  MSR_IA32_SYSENTER_ESP is set to 0, and
tss->sp0 is read from the percpu segment instead of relative to the
stack pointer.

--
Brian Gerst

[tip:x86/asm] x86/asm/entry/32: Document the 32-bit SYSENTER " emergency stack" better

[tip-bot for Denys Vlasenko]

Commit-ID:  9b41236f1032623ce0470481e97604fb5c5681fa
Gitweb:     http://git.kernel.org/tip/9b41236f1032623ce0470481e97604fb5c5681fa
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Mon, 9 Mar 2015 15:52:18 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 Mar 2015 11:05:37 +0100

x86/asm/entry/32: Document the 32-bit SYSENTER "emergency stack" better

Before the patch, the 'tss_struct::stack' field was not referenced anywhere.

It was used only to set SYSENTER's stack to point after the last byte
of tss_struct, thus the trailing field, stack[64], was used.

But grep would not know it. You can comment it out, compile,
and kernel will even run until an unlucky NMI corrupts
io_bitmap[] (which is also not easily detectable).

This patch changes code so that the purpose and usage of this
field is not mysterious anymore, and can be easily grepped for.

This does change generated code, for a subtle reason:
since tss_struct is ____cacheline_aligned, there happens to be
5 longs of padding at the end. Old code was using the padding
too; new code will strictly use it only for SYSENTER_stack[].

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425912738-559-2-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 4 ++--
 arch/x86/kernel/asm-offsets_32.c | 2 +-
 arch/x86/kernel/cpu/common.c     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index d157f94..5720997 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -291,9 +291,9 @@ struct tss_struct {
 	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
 
 	/*
-	 * .. and then another 0x100 bytes for the emergency kernel stack:
+	 * Space for the temporary SYSENTER stack:
 	 */
-	unsigned long		stack[64];
+	unsigned long		SYSENTER_stack[64];
 
 } ____cacheline_aligned;
 
diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
index 3b3b9d3..47703ae 100644
--- a/arch/x86/kernel/asm-offsets_32.c
+++ b/arch/x86/kernel/asm-offsets_32.c
@@ -68,7 +68,7 @@ void foo(void)
 
 	/* Offset from the sysenter stack to tss.sp0 */
 	DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
-		 sizeof(struct tss_struct));
+	       offsetofend(struct tss_struct, SYSENTER_stack));
 
 #if defined(CONFIG_LGUEST) || defined(CONFIG_LGUEST_GUEST) || defined(CONFIG_LGUEST_MODULE)
 	BLANK();
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 7634833..7a3dfb1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -987,7 +987,7 @@ void enable_sep_cpu(void)
 	}
 
 	tss->x86_tss.ss1 = __KERNEL_CS;
-	tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
+	tss->x86_tss.sp1 = (unsigned long)tss + offsetofend(struct tss_struct, SYSENTER_stack);
 	wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 	wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
 	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);

[tip:x86/asm] x86/asm/entry/32: Document the 32-bit SYSENTER " emergency stack" better

[tip-bot for Denys Vlasenko]

Commit-ID:  d828c71fba8922b116b4ec56c3e5bca8c822d5ae
Gitweb:     http://git.kernel.org/tip/d828c71fba8922b116b4ec56c3e5bca8c822d5ae
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Mon, 9 Mar 2015 15:52:18 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 Mar 2015 09:25:29 +0100

x86/asm/entry/32: Document the 32-bit SYSENTER "emergency stack" better

Before the patch, the 'tss_struct::stack' field was not referenced anywhere.

It was used only to set SYSENTER's stack to point after the last byte
of tss_struct, thus the trailing field, stack[64], was used.

But grep would not know it. You can comment it out, compile,
and kernel will even run until an unlucky NMI corrupts
io_bitmap[] (which is also not easily detectable).

This patch changes code so that the purpose and usage of this
field is not mysterious anymore, and can be easily grepped for.

This does change generated code, for a subtle reason:
since tss_struct is ____cacheline_aligned, there happens to be
5 longs of padding at the end. Old code was using the padding
too; new code will strictly use it only for SYSENTER_stack[].

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425912738-559-2-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 4 ++--
 arch/x86/kernel/asm-offsets_32.c | 2 +-
 arch/x86/kernel/cpu/common.c     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 6a5c0ec..5abd9a5 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -291,9 +291,9 @@ struct tss_struct {
 	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
 
 	/*
-	 * .. and then another 0x100 bytes for the emergency kernel stack:
+	 * Space for the temporary SYSENTER stack:
 	 */
-	unsigned long		stack[64];
+	unsigned long		SYSENTER_stack[64];
 
 } ____cacheline_aligned;
 
diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
index 3b3b9d3..47703ae 100644
--- a/arch/x86/kernel/asm-offsets_32.c
+++ b/arch/x86/kernel/asm-offsets_32.c
@@ -68,7 +68,7 @@ void foo(void)
 
 	/* Offset from the sysenter stack to tss.sp0 */
 	DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
-		 sizeof(struct tss_struct));
+	       offsetofend(struct tss_struct, SYSENTER_stack));
 
 #if defined(CONFIG_LGUEST) || defined(CONFIG_LGUEST_GUEST) || defined(CONFIG_LGUEST_MODULE)
 	BLANK();
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 7634833..7a3dfb1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -987,7 +987,7 @@ void enable_sep_cpu(void)
 	}
 
 	tss->x86_tss.ss1 = __KERNEL_CS;
-	tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
+	tss->x86_tss.sp1 = (unsigned long)tss + offsetofend(struct tss_struct, SYSENTER_stack);
 	wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 	wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
 	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> Suggested by Linus.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  include/linux/stddef.h |  9 +++++++++
>  include/linux/vfio.h   | 13 -------------
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/stddef.h b/include/linux/stddef.h
> index f4aec0e..076af43 100644
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef.h
> @@ -19,3 +19,12 @@ enum {
>  #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
>  #endif
>  #endif
> +
> +/**
> + * offsetofend(TYPE, MEMBER)
> + *
> + * @TYPE: The type of the structure
> + * @MEMBER: The member within the structure to get the end offset of
> + */
> +#define offsetofend(TYPE, MEMBER) \
> +	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 2d67b89..049b2f4 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -78,19 +78,6 @@ extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
>  extern void vfio_unregister_iommu_driver(
>  				const struct vfio_iommu_driver_ops *ops);
>  
> -/**
> - * offsetofend(TYPE, MEMBER)
> - *
> - * @TYPE: The type of the structure
> - * @MEMBER: The member within the structure to get the end offset of
> - *
> - * Simple helper macro for dealing with variable sized structures passed
> - * from user space.  This allows us to easily determine if the provided
> - * structure is sized to include various fields.
> - */
> -#define offsetofend(TYPE, MEMBER) \
> -	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))

So I like it, and because it is not particularly trivial when to use 
this primitive it was explained nicely in a description in the vfio.h 
version.

But you lost that nice description during the code move!!

Please don't.

Thanks,

	Ingo

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Denys Vlasenko]

On Mon, Mar 9, 2015 at 3:58 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> -/**
>> - * offsetofend(TYPE, MEMBER)
>> - *
>> - * @TYPE: The type of the structure
>> - * @MEMBER: The member within the structure to get the end offset of
>> - *
>> - * Simple helper macro for dealing with variable sized structures passed
>> - * from user space.  This allows us to easily determine if the provided
>> - * structure is sized to include various fields.
>> - */
>> -#define offsetofend(TYPE, MEMBER) \
>> -     (offsetof(TYPE, MEMBER) + sizeof(((TYPE *)0)->MEMBER))
>
> So I like it, and because it is not particularly trivial when to use
> this primitive it was explained nicely in a description in the vfio.h
> version.
>
> But you lost that nice description during the code move!!

That description was clearly specific to how that macro is used in
drivers/vfio/*.c, along the lines of

                minsz = offsetofend(struct vfio_eeh_pe_op, op);
                if (copy_from_user(&op, (void __user *)arg, minsz))
                        return -EFAULT;
                if (op.argsz < minsz || op.flags)
                        return -EINVAL;

But the macro is generic, it has many other uses besides this one.

Nevertheless, I can resend a version where comment survives if you want...

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Ingo Molnar]

* Denys Vlasenko <vda.linux@googlemail.com> wrote:

> On Mon, Mar 9, 2015 at 3:58 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >> -/**
> >> - * offsetofend(TYPE, MEMBER)
> >> - *
> >> - * @TYPE: The type of the structure
> >> - * @MEMBER: The member within the structure to get the end offset of
> >> - *
> >> - * Simple helper macro for dealing with variable sized structures passed
> >> - * from user space.  This allows us to easily determine if the provided
> >> - * structure is sized to include various fields.
> >> - */
> >> -#define offsetofend(TYPE, MEMBER) \
> >> -     (offsetof(TYPE, MEMBER) + sizeof(((TYPE *)0)->MEMBER))
> >
> > So I like it, and because it is not particularly trivial when to use
> > this primitive it was explained nicely in a description in the vfio.h
> > version.
> >
> > But you lost that nice description during the code move!!
> 
> That description was clearly specific to how that macro is used in
> drivers/vfio/*.c, along the lines of
> 
>                 minsz = offsetofend(struct vfio_eeh_pe_op, op);

Hm, but here 'minsz' == sizeof(struct vfio_eeh_pe_op), so the vfio 
usage does not seem to be justified.

>                 if (copy_from_user(&op, (void __user *)arg, minsz))
>                         return -EFAULT;
>                 if (op.argsz < minsz || op.flags)
>                         return -EINVAL;
> 
> But the macro is generic, it has many other uses besides this one.

So I might be missing something, but what generic uses does it have, 
beyond structures that have some rare size related weirdness, such as 
alignment attributes? In 99% of the cases:

   sizeof(struct) == offsetofend(struct, last_member)

right?

> Nevertheless, I can resend a version where comment survives if you 
> want...

So maybe extend it to a description that you think describes its uses 
correctly? People will keep wondering about when to use this.

Thanks,

	Ingo

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Andy Lutomirski]

On Mon, Mar 9, 2015 at 8:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Denys Vlasenko <vda.linux@googlemail.com> wrote:
>
>> On Mon, Mar 9, 2015 at 3:58 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> -/**
>> >> - * offsetofend(TYPE, MEMBER)
>> >> - *
>> >> - * @TYPE: The type of the structure
>> >> - * @MEMBER: The member within the structure to get the end offset of
>> >> - *
>> >> - * Simple helper macro for dealing with variable sized structures passed
>> >> - * from user space.  This allows us to easily determine if the provided
>> >> - * structure is sized to include various fields.
>> >> - */
>> >> -#define offsetofend(TYPE, MEMBER) \
>> >> -     (offsetof(TYPE, MEMBER) + sizeof(((TYPE *)0)->MEMBER))
>> >
>> > So I like it, and because it is not particularly trivial when to use
>> > this primitive it was explained nicely in a description in the vfio.h
>> > version.
>> >
>> > But you lost that nice description during the code move!!
>>
>> That description was clearly specific to how that macro is used in
>> drivers/vfio/*.c, along the lines of
>>
>>                 minsz = offsetofend(struct vfio_eeh_pe_op, op);
>
> Hm, but here 'minsz' == sizeof(struct vfio_eeh_pe_op), so the vfio
> usage does not seem to be justified.
>
>>                 if (copy_from_user(&op, (void __user *)arg, minsz))
>>                         return -EFAULT;
>>                 if (op.argsz < minsz || op.flags)
>>                         return -EINVAL;
>>
>> But the macro is generic, it has many other uses besides this one.
>
> So I might be missing something, but what generic uses does it have,
> beyond structures that have some rare size related weirdness, such as
> alignment attributes? In 99% of the cases:
>
>    sizeof(struct) == offsetofend(struct, last_member)
>
> right?

struct foo {
    u64 a;
    char b;
};

sizeof(struct foo) will be 16, but offsetofend(struct foo, b) will be
9 on most platforms, right?

--Andy

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> > So I might be missing something, but what generic uses does it 
> > have, beyond structures that have some rare size related 
> > weirdness, such as alignment attributes? In 99% of the cases:
> >
> >    sizeof(struct) == offsetofend(struct, last_member)
> >
> > right?
> 
> struct foo {
>     u64 a;
>     char b;
> };
> 
> sizeof(struct foo) will be 16, but offsetofend(struct foo, b) will be
> 9 on most platforms, right?

I knew I missed something obvious :-)

Let me attempt to get it right:

When the next byte after the last member of a structure is not aligned 
to the largest alignment requirement of any structure member, then the 
structure grows (is padded) and offsetofend() < sizeof().

'packed' or 'aligned' attributes will modify the largest alignment 
requirement value so they are a common but not only mechanism for this 
to be the case.

Thanks,

	Ingo

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Alex Williamson]

On Mon, 2015-03-09 at 16:28 +0100, Ingo Molnar wrote:
> * Denys Vlasenko <vda.linux@googlemail.com> wrote:
> 
> > On Mon, Mar 9, 2015 at 3:58 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > >> -/**
> > >> - * offsetofend(TYPE, MEMBER)
> > >> - *
> > >> - * @TYPE: The type of the structure
> > >> - * @MEMBER: The member within the structure to get the end offset of
> > >> - *
> > >> - * Simple helper macro for dealing with variable sized structures passed
> > >> - * from user space.  This allows us to easily determine if the provided
> > >> - * structure is sized to include various fields.
> > >> - */
> > >> -#define offsetofend(TYPE, MEMBER) \
> > >> -     (offsetof(TYPE, MEMBER) + sizeof(((TYPE *)0)->MEMBER))
> > >
> > > So I like it, and because it is not particularly trivial when to use
> > > this primitive it was explained nicely in a description in the vfio.h
> > > version.
> > >
> > > But you lost that nice description during the code move!!
> > 
> > That description was clearly specific to how that macro is used in
> > drivers/vfio/*.c, along the lines of
> > 
> >                 minsz = offsetofend(struct vfio_eeh_pe_op, op);
> 
> Hm, but here 'minsz' == sizeof(struct vfio_eeh_pe_op), so the vfio 
> usage does not seem to be justified.
> 
> >                 if (copy_from_user(&op, (void __user *)arg, minsz))
> >                         return -EFAULT;
> >                 if (op.argsz < minsz || op.flags)
> >                         return -EINVAL;
> > 
> > But the macro is generic, it has many other uses besides this one.
> 
> So I might be missing something, but what generic uses does it have, 
> beyond structures that have some rare size related weirdness, such as 
> alignment attributes? In 99% of the cases:
> 
>    sizeof(struct) == offsetofend(struct, last_member)
> 
> right?

The idea in the vfio code is to allow the structure to evolve over time
while maintaining compatibility.  We effectively create a header in the
structure with the base functionality and flags and structure size can
tell us what optional fields are present.  A better examples is
drivers/vfio/pci/vfio_pci.c:vfio_pci_ioctl(), particularly
VFIO_DEVICE_SET_IRQS where we read what's being provided in the "header"
and then read beyond the minimum structure size if directed.  Thanks,

Alex

> > Nevertheless, I can resend a version where comment survives if you 
> > want...
> 
> So maybe extend it to a description that you think describes its uses 
> correctly? People will keep wondering about when to use this.
> 
> Thanks,
> 
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] move offsetofend() from vfio.h to stddef.h

[Linus Torvalds]

On Mon, Mar 9, 2015 at 7:52 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Suggested by Linus.

Actually, that was Andy, not me.

But I have nothing against it.

                       Linus

[tip:x86/asm] include/stddef.h: Move offsetofend() from vfio.h to a generic kernel header

[tip-bot for Denys Vlasenko]

Commit-ID:  23ec13ebc0426fc5f3ecc623c432d57f919e1b28
Gitweb:     http://git.kernel.org/tip/23ec13ebc0426fc5f3ecc623c432d57f919e1b28
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Mon, 9 Mar 2015 15:52:17 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 Mar 2015 11:05:37 +0100

include/stddef.h: Move offsetofend() from vfio.h to a generic kernel header

Suggested by Andy.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425912738-559-1-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/stddef.h |  9 +++++++++
 include/linux/vfio.h   | 13 -------------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index f4aec0e..076af43 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -19,3 +19,12 @@ enum {
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
 #endif
+
+/**
+ * offsetofend(TYPE, MEMBER)
+ *
+ * @TYPE: The type of the structure
+ * @MEMBER: The member within the structure to get the end offset of
+ */
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 2d67b89..049b2f4 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -78,19 +78,6 @@ extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
 extern void vfio_unregister_iommu_driver(
 				const struct vfio_iommu_driver_ops *ops);
 
-/**
- * offsetofend(TYPE, MEMBER)
- *
- * @TYPE: The type of the structure
- * @MEMBER: The member within the structure to get the end offset of
- *
- * Simple helper macro for dealing with variable sized structures passed
- * from user space.  This allows us to easily determine if the provided
- * structure is sized to include various fields.
- */
-#define offsetofend(TYPE, MEMBER) \
-	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
-
 /*
  * External user API
  */

[tip:x86/asm] include/stddef.h: Move offsetofend() from vfio.h to a generic kernel header

[tip-bot for Denys Vlasenko]

Commit-ID:  3876488444e71238e287459c39d7692b6f718c3e
Gitweb:     http://git.kernel.org/tip/3876488444e71238e287459c39d7692b6f718c3e
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Mon, 9 Mar 2015 15:52:17 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 Mar 2015 09:25:28 +0100

include/stddef.h: Move offsetofend() from vfio.h to a generic kernel header

Suggested by Andy.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425912738-559-1-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/stddef.h |  9 +++++++++
 include/linux/vfio.h   | 13 -------------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index f4aec0e..076af43 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -19,3 +19,12 @@ enum {
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
 #endif
+
+/**
+ * offsetofend(TYPE, MEMBER)
+ *
+ * @TYPE: The type of the structure
+ * @MEMBER: The member within the structure to get the end offset of
+ */
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 2d67b89..049b2f4 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -78,19 +78,6 @@ extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
 extern void vfio_unregister_iommu_driver(
 				const struct vfio_iommu_driver_ops *ops);
 
-/**
- * offsetofend(TYPE, MEMBER)
- *
- * @TYPE: The type of the structure
- * @MEMBER: The member within the structure to get the end offset of
- *
- * Simple helper macro for dealing with variable sized structures passed
- * from user space.  This allows us to easily determine if the provided
- * structure is sized to include various fields.
- */
-#define offsetofend(TYPE, MEMBER) \
-	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
-
 /*
  * External user API
  */