[PATCH 0/2] Removing the use of VLAIS from the Linux Kernel

[PATCH 0/2] Removing the use of VLAIS from the Linux Kernel

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). The LLVMLinux Project is working towards the ability of
providing the Linux kernel developer the choice of using the Clang compiler
toolchain. This is a part of a series of patches which remove the use of VLAIS
from crypto code, dm-crypt, jbd2, libcrc32c, netfilter, and usb gadget. Other
patches to allow Clang to be used will follow.

Behan Webster (2):
  Helper macros used for replacing the use of VLAIS
  Remove VLAIS usage from gadget code

 drivers/usb/gadget/f_fs.c |  106 ++++++++++++++++++++++++++-------------------
 include/linux/valign.h    |   87 +++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 45 deletions(-)
 create mode 100644 include/linux/valign.h

-- 
1.7.9.5

[PATCH 1/2] Helper macros used for replacing the use of VLAIS

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This new header file contains macros which can be used to
calculate the size and offset of variables in an allocated buffer of memory
taking into account alignment issues.

Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 include/linux/valign.h |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 include/linux/valign.h

diff --git a/include/linux/valign.h b/include/linux/valign.h
new file mode 100644
index 0000000..b39381b
--- /dev/null
+++ b/include/linux/valign.h
@@ -0,0 +1,87 @@
+/*
+ * Variable alignment macros used to break up a larger chunk of memory into
+ * smaller variables. Meant to be used to replace the use of Variable Length
+ * Arrays In Structures (VLAIS)
+ *
+ *  Copyright (C) 2012 Behan Webster <behanw@conversincode.com>
+ */
+
+#ifndef _VALIGN_H_
+#define _VALIGN_H_
+
+/**
+ * truncalign() - Align a memory address by truncation
+ * @num:	Address or size to align
+ * @padwidth:	Number of byte upon which to align
+ *
+ * Truncate an address or size to a particular memory alignment.
+ * Used by truncalign().
+ */
+#define truncalign(num, padwidth) ((long)(num) & ~((padwidth)-1))
+
+/**
+ * padalign() - Align a memory address by padding
+ * @num:	Address or size to align
+ * @padwidth:	Number of byte upon which to align
+ *
+ * Pad out an address or size to a particular memory alignment
+ * Used by paddedsize() and paddedstart().
+ */
+#define padalign(num, padwidth) \
+	truncalign((long)(num) + ((padwidth)-1), padwidth)
+
+/**
+ * paddedsize() - Calculate the size of an chunk of aligned memory
+ * @offset:	Unaligned offset to the start of the chunk size being calculated
+ * @num:	The number of variables in the array of "type" (can be 1)
+ * @type:	The type of variables in the array
+ * @nexttype:	The type of the next variable in the large piece of memory
+ *
+ * Calculate the size that a variable (or array) will take as a part of a
+ * larger piece of memory.  Takes into account a potentially unaligned offset
+ * into the larger piece of allocated memory, the alignment of the variable
+ * type, and the alignement of the type of the variable to be used after that.
+ *
+ * Example: size_t l = paddedsize(1, 2, short, int);
+ *
+ * The example above would give you a padded size of 6 bytes: 2x 16-bit shorts,
+ * starting at 2 bytes into the buffer (the offset of 1 byte being padded out
+ * to 2 bytes) followed by 2 bytes of padding so that the next type (a 32-bit
+ * int) would be 32-bit aligned. looking like this:
+ *
+ *   0: O.SS SS.. iiii
+ *        \-----/ <-- 2 shorts + 2 bytes of padding = size of 6 bytes
+ *
+ * O = The offset
+ * . = Padding bytes
+ * S = 2 shorts
+ * i = int which will theoretically be next
+ */
+#define paddedsize(offset, num, type, nexttype) (padalign((offset) \
+	+ (num) * sizeof(type), __alignof__(nexttype)) - (offset))
+
+/**
+ * paddedstart() - Calculate the start of a chunk of aligned memory
+ * @ptr:	Pointer from which to calculate the start of the chunk
+ * @offset:	Offset from the ptr to the start of the chunk being calculated
+ * @type:	The type of variable in the chunk of memory
+ *
+ * Calculate the start address of a variable based on the offset from an
+ * address, aligned based on the type of the variable specified.
+ *
+ * Example: char *data = kmalloc(size, GFP_KERNEL);
+ *          long *var = paddedstart(data, 12, long);
+ *
+ * The example above on a 64-bit machine would return the equivalent of
+ * &buffer[16] since a long needs to be 8 byte aligned.
+ *
+ *   0: OOOO OOOO OOOO .... LLLL LLLL
+ *                          ^ <-- The start address of the long
+ * O = The offset
+ * . = Padding bytes
+ * L = The long
+ */
+#define paddedstart(ptr, offset, type) \
+	(type *)padalign((long)(ptr)+(offset), __alignof__(type))
+
+#endif
-- 
1.7.9.5

Re: [PATCH 1/2] Helper macros used for replacing the use of VLAIS

[Greg KH]

On Tue, Oct 30, 2012 at 02:25:19PM -0400, Behan Webster wrote:
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This new header file contains macros which can be used to
> calculate the size and offset of variables in an allocated buffer of memory
> taking into account alignment issues.
> 
> Signed-off-by: Behan Webster <behanw@converseincode.com>
> ---
>  include/linux/valign.h |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 include/linux/valign.h

You sent the same patch here, to two different trees (usb and
networking).  That's not good, what happens if they both apply them?

thanks,

greg k-h

Re: [PATCH 1/2] Helper macros used for replacing the use of VLAIS

[Behan Webster]

On 12-10-30 03:20 PM, Greg KH wrote:
> On Tue, Oct 30, 2012 at 02:25:19PM -0400, Behan Webster wrote:
>> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
>> precludes the use of compilers which don't implement VLAIS (for instance the
>> Clang compiler). This new header file contains macros which can be used to
>> calculate the size and offset of variables in an allocated buffer of memory
>> taking into account alignment issues.
>>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
>> ---
>>   include/linux/valign.h |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 87 insertions(+)
>>   create mode 100644 include/linux/valign.h
> You sent the same patch here, to two different trees (usb and
> networking).  That's not good, what happens if they both apply them?
Good point. My mistake.

People in both subsystems needed to see the patch which is why I sent them.

What would have been a better approach? And how can I fix this?

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH 1/2] Helper macros used for replacing the use of VLAIS

[Greg KH]

On Tue, Oct 30, 2012 at 03:35:24PM -0400, Behan Webster wrote:
> On 12-10-30 03:20 PM, Greg KH wrote:
> >On Tue, Oct 30, 2012 at 02:25:19PM -0400, Behan Webster wrote:
> >>The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> >>precludes the use of compilers which don't implement VLAIS (for instance the
> >>Clang compiler). This new header file contains macros which can be used to
> >>calculate the size and offset of variables in an allocated buffer of memory
> >>taking into account alignment issues.
> >>
> >>Signed-off-by: Behan Webster <behanw@converseincode.com>
> >>---
> >>  include/linux/valign.h |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 87 insertions(+)
> >>  create mode 100644 include/linux/valign.h
> >You sent the same patch here, to two different trees (usb and
> >networking).  That's not good, what happens if they both apply them?
> Good point. My mistake.
> 
> People in both subsystems needed to see the patch which is why I sent them.
> 
> What would have been a better approach? And how can I fix this?

Send all 3 as a single series, to everyone and ask the different
maintainers to work together to figure out who is going to apply what.

Yeah, it's tough, but things like this are pretty rare, so they don't
come up often.

greg k-h

[PATCH 2/2] Remove VLAIS usage from gadget code

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch instead calculates offsets into the kmalloc-ed
memory buffer using macros from valign.h.

Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 drivers/usb/gadget/f_fs.c |  106 ++++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 64c4ec1..ca6cacc 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -22,6 +22,7 @@
 #include <linux/pagemap.h>
 #include <linux/export.h>
 #include <linux/hid.h>
+#include <linux/valign.h>
 #include <asm/unaligned.h>
 
 #include <linux/usb/composite.h>
@@ -1875,6 +1876,15 @@ error:
 	return ret;
 }
 
+static void __ffs_init_stringtabs(struct usb_gadget_strings **stringtabs,
+	struct usb_gadget_strings *t, unsigned len)
+{
+		do {
+			*stringtabs++ = t++;
+		} while (--len);
+		*stringtabs = NULL;
+}
+
 static int __ffs_data_got_strings(struct ffs_data *ffs,
 				  char *const _data, size_t len)
 {
@@ -1911,31 +1921,26 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
 
 	/* Allocate everything in one chunk so there's less maintenance. */
 	{
-		struct {
-			struct usb_gadget_strings *stringtabs[lang_count + 1];
-			struct usb_gadget_strings stringtab[lang_count];
-			struct usb_string strings[lang_count*(needed_count+1)];
-		} *d;
-		unsigned i = 0;
-
-		d = kmalloc(sizeof *d, GFP_KERNEL);
+		int strtabslen = paddedsize(0, lang_count+1,
+			struct usb_gadget_strings *, struct usb_gadget_strings);
+		int strtablen = paddedsize(strtabslen, lang_count,
+			struct usb_gadget_strings, struct usb_string);
+		int strlen = paddedsize(strtabslen + strtablen,
+			lang_count*(needed_count+1), struct usb_string, int);
+
+		stringtabs = kmalloc(strtabslen + strtablen + strlen,
+			GFP_KERNEL);
 		if (unlikely(!d)) {
 			kfree(_data);
 			return -ENOMEM;
 		}
 
-		stringtabs = d->stringtabs;
-		t = d->stringtab;
-		i = lang_count;
-		do {
-			*stringtabs++ = t++;
-		} while (--i);
-		*stringtabs = NULL;
-
-		stringtabs = d->stringtabs;
-		t = d->stringtab;
-		s = d->strings;
+		t = paddedstart(stringtabs, strtabslen,
+			struct usb_gadget_strings);
+		s = paddedstart(t, strtablen, struct usb_string);
 		strings = s;
+
+		__ffs_init_stringtabs(stringtabs, t, lang_count);
 	}
 
 	/* For each language */
@@ -2209,17 +2214,23 @@ static int ffs_func_bind(struct usb_configuration *c,
 
 	int ret;
 
-	/* Make it a single chunk, less management later on */
-	struct {
-		struct ffs_ep eps[ffs->eps_count];
-		struct usb_descriptor_header
-			*fs_descs[full ? ffs->fs_descs_count + 1 : 0];
-		struct usb_descriptor_header
-			*hs_descs[high ? ffs->hs_descs_count + 1 : 0];
-		short inums[ffs->interfaces_count];
-		char raw_descs[high ? ffs->raw_descs_length
-				    : ffs->raw_fs_descs_length];
-	} *data;
+	struct ffs_ep *eps;
+	struct usb_descriptor_header **fs_descs;
+	struct usb_descriptor_header **hs_descs;
+	short *inums;
+	char *raw_descs;
+
+	int epslen = paddedsize(0, ffs->eps_count,
+			struct ffs_ep, struct usb_descriptor_header);
+	int fsdlen = paddedsize(epslen, full ? ffs->fs_descs_count + 1 : 0,
+			struct usb_descriptor_header,
+			struct usb_descriptor_header);
+	int hsdlen = paddedsize(fsdlen, high ? ffs->hs_descs_count + 1 : 0,
+			struct usb_descriptor_header, short);
+	int inumlen = paddedsize(hsdlen, ffs->interfaces_count, short, char);
+	int rawlen = paddedsize(inumlen,
+		high ? ffs->raw_descs_length : ffs->raw_fs_descs_length,
+		char, int);
 
 	ENTER();
 
@@ -2227,21 +2238,28 @@ static int ffs_func_bind(struct usb_configuration *c,
 	if (unlikely(!(full | high)))
 		return -ENOTSUPP;
 
-	/* Allocate */
-	data = kmalloc(sizeof *data, GFP_KERNEL);
+	/* Allocate a single chunk, less management later on */
+	eps = kmalloc(epslen+fsdlen+hsdlen+inumlen+rawlen, GFP_KERNEL);
 	if (unlikely(!data))
 		return -ENOMEM;
 
+	/* Calculate start of each variable in the allocated memory */
+	fs_descs = paddedstart(eps, epslen, struct usb_descriptor_header *);
+	hs_descs = paddedstart(fs_descs, fsdlen,
+		struct usb_descriptor_header *);
+	inums = paddedstart(hs_descs, hsdlen, short);
+	raw_descs = paddedstart(inums, inumlen, char);
+
 	/* Zero */
-	memset(data->eps, 0, sizeof data->eps);
-	memcpy(data->raw_descs, ffs->raw_descs + 16, sizeof data->raw_descs);
-	memset(data->inums, 0xff, sizeof data->inums);
+	memset(eps, 0, epslen);
+	memcpy(raw_descs, ffs->raw_descs + 16, rawlen);
+	memset(inums, 0xff, inumlen);
 	for (ret = ffs->eps_count; ret; --ret)
-		data->eps[ret].num = -1;
+		eps[ret].num = -1;
 
 	/* Save pointers */
-	func->eps             = data->eps;
-	func->interfaces_nums = data->inums;
+	func->eps             = eps;
+	func->interfaces_nums = inums;
 
 	/*
 	 * Go through all the endpoint descriptors and allocate
@@ -2249,10 +2267,9 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 * numbers without worrying that it may be described later on.
 	 */
 	if (likely(full)) {
-		func->function.descriptors = data->fs_descs;
+		func->function.descriptors = fs_descs;
 		ret = ffs_do_descs(ffs->fs_descs_count,
-				   data->raw_descs,
-				   sizeof data->raw_descs,
+				   raw_descs, rawlen,
 				   __ffs_func_bind_do_descs, func);
 		if (unlikely(ret < 0))
 			goto error;
@@ -2261,10 +2278,9 @@ static int ffs_func_bind(struct usb_configuration *c,
 	}
 
 	if (likely(high)) {
-		func->function.hs_descriptors = data->hs_descs;
+		func->function.hs_descriptors = hs_descs;
 		ret = ffs_do_descs(ffs->hs_descs_count,
-				   data->raw_descs + ret,
-				   (sizeof data->raw_descs) - ret,
+				   raw_descs + ret, rawlen - ret,
 				   __ffs_func_bind_do_descs, func);
 	}
 
@@ -2275,7 +2291,7 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 */
 	ret = ffs_do_descs(ffs->fs_descs_count +
 			   (high ? ffs->hs_descs_count : 0),
-			   data->raw_descs, sizeof data->raw_descs,
+			   raw_descs, rawlen,
 			   __ffs_func_bind_do_nums, func);
 	if (unlikely(ret < 0))
 		goto error;
-- 
1.7.9.5

Re: [PATCH 2/2] Remove VLAIS usage from gadget code

[Joseph Glanville]

On 31 October 2012 05:25, Behan Webster <behanw@converseincode.com> wrote:
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch instead calculates offsets into the kmalloc-ed
> memory buffer using macros from valign.h.
>
> Signed-off-by: Behan Webster <behanw@converseincode.com>

Linux only lists 2 requirements in the README.

1. GCC
2. PMMU ( granted this implies a much bigger list of requirements. :P )

Deficiences in Clangs GCC compatibility mode should be addressed
rather than pointlessly changing kernel code.
Possibly propose a patch to Clang instead?

Joseph.

-- 
CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

[PATCH 0/2] Removing the use of VLAIS from netfilter

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). The LLVMLinux Project is working towards the ability of
providing the Linux kernel developer the choice of using the Clang compiler
toolchain. This is a part of a series of patches which remove the use of VLAIS
from crypto code, dm-crypt, jbd2, libcrc32c, netfilter, and usb gadget. Other
patches to allow Clang to be used will follow.

Behan Webster (1):
  Helper macros used for replacing the use of VLAIS

Jan-Simon Möller (1):
  Remove VLAIS usage from netfilter

 include/linux/valign.h      |   87 +++++++++++++++++++++++++++++++++++++++++++
 net/netfilter/xt_repldata.h |   40 ++++++++++++--------
 2 files changed, 111 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/valign.h

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] Helper macros used for replacing the use of VLAIS

[Behan Webster]

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This new header file contains macros which can be used to
calculate the size and offset of variables in an allocated buffer of memory
taking into account alignment issues.

Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 include/linux/valign.h |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 include/linux/valign.h

diff --git a/include/linux/valign.h b/include/linux/valign.h
new file mode 100644
index 0000000..b39381b
--- /dev/null
+++ b/include/linux/valign.h
@@ -0,0 +1,87 @@
+/*
+ * Variable alignment macros used to break up a larger chunk of memory into
+ * smaller variables. Meant to be used to replace the use of Variable Length
+ * Arrays In Structures (VLAIS)
+ *
+ *  Copyright (C) 2012 Behan Webster <behanw@conversincode.com>
+ */
+
+#ifndef _VALIGN_H_
+#define _VALIGN_H_
+
+/**
+ * truncalign() - Align a memory address by truncation
+ * @num:	Address or size to align
+ * @padwidth:	Number of byte upon which to align
+ *
+ * Truncate an address or size to a particular memory alignment.
+ * Used by truncalign().
+ */
+#define truncalign(num, padwidth) ((long)(num) & ~((padwidth)-1))
+
+/**
+ * padalign() - Align a memory address by padding
+ * @num:	Address or size to align
+ * @padwidth:	Number of byte upon which to align
+ *
+ * Pad out an address or size to a particular memory alignment
+ * Used by paddedsize() and paddedstart().
+ */
+#define padalign(num, padwidth) \
+	truncalign((long)(num) + ((padwidth)-1), padwidth)
+
+/**
+ * paddedsize() - Calculate the size of an chunk of aligned memory
+ * @offset:	Unaligned offset to the start of the chunk size being calculated
+ * @num:	The number of variables in the array of "type" (can be 1)
+ * @type:	The type of variables in the array
+ * @nexttype:	The type of the next variable in the large piece of memory
+ *
+ * Calculate the size that a variable (or array) will take as a part of a
+ * larger piece of memory.  Takes into account a potentially unaligned offset
+ * into the larger piece of allocated memory, the alignment of the variable
+ * type, and the alignement of the type of the variable to be used after that.
+ *
+ * Example: size_t l = paddedsize(1, 2, short, int);
+ *
+ * The example above would give you a padded size of 6 bytes: 2x 16-bit shorts,
+ * starting at 2 bytes into the buffer (the offset of 1 byte being padded out
+ * to 2 bytes) followed by 2 bytes of padding so that the next type (a 32-bit
+ * int) would be 32-bit aligned. looking like this:
+ *
+ *   0: O.SS SS.. iiii
+ *        \-----/ <-- 2 shorts + 2 bytes of padding = size of 6 bytes
+ *
+ * O = The offset
+ * . = Padding bytes
+ * S = 2 shorts
+ * i = int which will theoretically be next
+ */
+#define paddedsize(offset, num, type, nexttype) (padalign((offset) \
+	+ (num) * sizeof(type), __alignof__(nexttype)) - (offset))
+
+/**
+ * paddedstart() - Calculate the start of a chunk of aligned memory
+ * @ptr:	Pointer from which to calculate the start of the chunk
+ * @offset:	Offset from the ptr to the start of the chunk being calculated
+ * @type:	The type of variable in the chunk of memory
+ *
+ * Calculate the start address of a variable based on the offset from an
+ * address, aligned based on the type of the variable specified.
+ *
+ * Example: char *data = kmalloc(size, GFP_KERNEL);
+ *          long *var = paddedstart(data, 12, long);
+ *
+ * The example above on a 64-bit machine would return the equivalent of
+ * &buffer[16] since a long needs to be 8 byte aligned.
+ *
+ *   0: OOOO OOOO OOOO .... LLLL LLLL
+ *                          ^ <-- The start address of the long
+ * O = The offset
+ * . = Padding bytes
+ * L = The long
+ */
+#define paddedstart(ptr, offset, type) \
+	(type *)padalign((long)(ptr)+(offset), __alignof__(type))
+
+#endif
-- 
1.7.9.5