[PATCH v2 0/8] Add kvzalloc_struct to complement kvzalloc_array

[PATCH v2 0/8] Add kvzalloc_struct to complement kvzalloc_array

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

We all know the perils of multiplying a value provided from userspace
by a constant and then allocating the resulting number of bytes.  That's
why we have kvmalloc_array(), so we don't have to think about it.
This solves the same problem when we embed one of these arrays in a
struct like this:

struct {
        int n;
        unsigned long array[];
};

Using kvzalloc_struct() to allocate this will save precious thinking
time and reduce the possibilities of bugs.

v2: Minor fixes pointed out by Kees
    Added sample conversions

I added a few more sample conversions for demonstration purposes, and
one thing I noticed is that the kvmalloc family of functions live in
<linux/mm.h> which (contrary to popular belief) is not already
automatically included everywhere.

Should they be moved to slab.h to be with kmalloc, a new file
(malloc.h? kvmalloc.h?), or even kernel.h?

Matthew Wilcox (8):
  mm: Add kernel-doc for kvfree
  mm: Add kvmalloc_ab_c and kvzalloc_struct
  Convert virtio_console to kvzalloc_struct
  Convert dax device to kvzalloc_struct
  Convert infiniband uverbs to kvzalloc_struct
  Convert v4l2 event to kvzalloc_struct
  Convert vhost to kvzalloc_struct
  Convert jffs2 acl to kvzalloc_struct

 drivers/char/virtio_console.c        |  3 +--
 drivers/dax/device.c                 |  2 +-
 drivers/infiniband/core/uverbs_cmd.c |  4 +--
 drivers/media/v4l2-core/v4l2-event.c |  3 +--
 drivers/vhost/vhost.c                |  2 +-
 fs/jffs2/acl.c                       |  3 ++-
 fs/jffs2/acl.h                       |  1 +
 include/linux/mm.h                   | 51 ++++++++++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h              |  5 +---
 mm/util.c                            | 10 +++++++
 10 files changed, 71 insertions(+), 13 deletions(-)

-- 
2.15.1

[PATCH v2 0/8] Add kvzalloc_struct to complement kvzalloc_array

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

We all know the perils of multiplying a value provided from userspace
by a constant and then allocating the resulting number of bytes.  That's
why we have kvmalloc_array(), so we don't have to think about it.
This solves the same problem when we embed one of these arrays in a
struct like this:

struct {
        int n;
        unsigned long array[];
};

Using kvzalloc_struct() to allocate this will save precious thinking
time and reduce the possibilities of bugs.

v2: Minor fixes pointed out by Kees
    Added sample conversions

I added a few more sample conversions for demonstration purposes, and
one thing I noticed is that the kvmalloc family of functions live in
<linux/mm.h> which (contrary to popular belief) is not already
automatically included everywhere.

Should they be moved to slab.h to be with kmalloc, a new file
(malloc.h? kvmalloc.h?), or even kernel.h?

Matthew Wilcox (8):
  mm: Add kernel-doc for kvfree
  mm: Add kvmalloc_ab_c and kvzalloc_struct
  Convert virtio_console to kvzalloc_struct
  Convert dax device to kvzalloc_struct
  Convert infiniband uverbs to kvzalloc_struct
  Convert v4l2 event to kvzalloc_struct
  Convert vhost to kvzalloc_struct
  Convert jffs2 acl to kvzalloc_struct

 drivers/char/virtio_console.c        |  3 +--
 drivers/dax/device.c                 |  2 +-
 drivers/infiniband/core/uverbs_cmd.c |  4 +--
 drivers/media/v4l2-core/v4l2-event.c |  3 +--
 drivers/vhost/vhost.c                |  2 +-
 fs/jffs2/acl.c                       |  3 ++-
 fs/jffs2/acl.h                       |  1 +
 include/linux/mm.h                   | 51 ++++++++++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h              |  5 +---
 mm/util.c                            | 10 +++++++
 10 files changed, 71 insertions(+), 13 deletions(-)

-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 1/8] mm: Add kernel-doc for kvfree

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 mm/util.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/util.c b/mm/util.c
index c1250501364f..dc4c7b551aaf 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -430,6 +430,16 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
 }
 EXPORT_SYMBOL(kvmalloc_node);
 
+/**
+ * kvfree() - Free memory.
+ * @addr: Pointer to allocated memory.
+ *
+ * kvfree frees memory allocated by any of vmalloc(), kmalloc() or
+ * kvmalloc().  It is slightly more efficient to use kfree() or vfree()
+ * if you are certain that you know which one to use.
+ *
+ * Context: Any context except NMI.
+ */
 void kvfree(const void *addr)
 {
 	if (is_vmalloc_addr(addr))
-- 
2.15.1

[PATCH v2 1/8] mm: Add kernel-doc for kvfree

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 mm/util.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/util.c b/mm/util.c
index c1250501364f..dc4c7b551aaf 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -430,6 +430,16 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
 }
 EXPORT_SYMBOL(kvmalloc_node);
 
+/**
+ * kvfree() - Free memory.
+ * @addr: Pointer to allocated memory.
+ *
+ * kvfree frees memory allocated by any of vmalloc(), kmalloc() or
+ * kvmalloc().  It is slightly more efficient to use kfree() or vfree()
+ * if you are certain that you know which one to use.
+ *
+ * Context: Any context except NMI.
+ */
 void kvfree(const void *addr)
 {
 	if (is_vmalloc_addr(addr))
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

We have kvmalloc_array in order to safely allocate an array with a
number of elements specified by userspace (avoiding arithmetic overflow
leading to a buffer overrun).  But it's fairly common to have a header
in front of that array (eg specifying the length of the array), so we
need a helper function for that situation.

kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
of our best efforts to name the arguments, it's really hard to remember
which order to put the arguments in.  kvzalloc_struct() eliminates that
effort; you tell it about the struct you're allocating, and it puts the
arguments in the right order for you (and checks that the arguments
you've given are at least plausible).

For comparison between the three schemes:

	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
			GFP_KERNEL);
	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
			GFP_KERNEL);
	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 include/linux/mm.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 81bd7f0be286..3b07ba12c8cc 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
 	return kvmalloc(n * size, flags);
 }
 
+/**
+ * kvmalloc_ab_c() - Allocate (a * b + c) bytes of memory.
+ * @n: Number of elements.
+ * @size: Size of each element (should be constant).
+ * @c: Size of header (should be constant).
+ * @gfp: Memory allocation flags.
+ *
+ * Use this function to allocate @n * @size + @c bytes of memory.  This
+ * function is safe to use when @n is controlled from userspace; it will
+ * return %NULL if the required amount of memory cannot be allocated.
+ * Use kvfree() to free the allocated memory.
+ *
+ * The kvzalloc_struct() function is easier to use as it has typechecking
+ * and you do not need to remember which of the arguments should be constants.
+ *
+ * Context: Process context.  May sleep; the @gfp flags should be based on
+ *	    %GFP_KERNEL.
+ * Return: A pointer to the allocated memory or %NULL.
+ */
+static inline __must_check
+void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
+{
+	if (size != 0 && n > (SIZE_MAX - c) / size)
+		return NULL;
+
+	return kvmalloc(n * size + c, gfp);
+}
+#define kvzalloc_ab_c(a, b, c, gfp) kvmalloc_ab_c(a, b, c, (gfp) | __GFP_ZERO)
+
+/**
+ * kvzalloc_struct() - Allocate and zero-fill a structure containing a
+ *		       variable length array.
+ * @p: Pointer to the structure.
+ * @member: Name of the array member.
+ * @n: Number of elements in the array.
+ * @gfp: Memory allocation flags.
+ *
+ * Allocate (and zero-fill) enough memory for a structure with an array
+ * of @n elements.  This function is safe to use when @n is specified by
+ * userspace as the arithmetic will not overflow.
+ * Use kvfree() to free the allocated memory.
+ *
+ * Context: Process context.  May sleep; the @gfp flags should be based on
+ *	    %GFP_KERNEL.
+ * Return: Zero-filled memory or a NULL pointer.
+ */
+#define kvzalloc_struct(p, member, n, gfp)				\
+	(typeof(p))kvzalloc_ab_c(n,					\
+		sizeof(*(p)->member) + __must_be_array((p)->member),	\
+		offsetof(typeof(*(p)), member), gfp)
+
 extern void kvfree(const void *addr);
 
 static inline atomic_t *compound_mapcount_ptr(struct page *page)
-- 
2.15.1

[PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

We have kvmalloc_array in order to safely allocate an array with a
number of elements specified by userspace (avoiding arithmetic overflow
leading to a buffer overrun).  But it's fairly common to have a header
in front of that array (eg specifying the length of the array), so we
need a helper function for that situation.

kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
of our best efforts to name the arguments, it's really hard to remember
which order to put the arguments in.  kvzalloc_struct() eliminates that
effort; you tell it about the struct you're allocating, and it puts the
arguments in the right order for you (and checks that the arguments
you've given are at least plausible).

For comparison between the three schemes:

	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
			GFP_KERNEL);
	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
			GFP_KERNEL);
	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 include/linux/mm.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 81bd7f0be286..3b07ba12c8cc 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
 	return kvmalloc(n * size, flags);
 }
 
+/**
+ * kvmalloc_ab_c() - Allocate (a * b + c) bytes of memory.
+ * @n: Number of elements.
+ * @size: Size of each element (should be constant).
+ * @c: Size of header (should be constant).
+ * @gfp: Memory allocation flags.
+ *
+ * Use this function to allocate @n * @size + @c bytes of memory.  This
+ * function is safe to use when @n is controlled from userspace; it will
+ * return %NULL if the required amount of memory cannot be allocated.
+ * Use kvfree() to free the allocated memory.
+ *
+ * The kvzalloc_struct() function is easier to use as it has typechecking
+ * and you do not need to remember which of the arguments should be constants.
+ *
+ * Context: Process context.  May sleep; the @gfp flags should be based on
+ *	    %GFP_KERNEL.
+ * Return: A pointer to the allocated memory or %NULL.
+ */
+static inline __must_check
+void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
+{
+	if (size != 0 && n > (SIZE_MAX - c) / size)
+		return NULL;
+
+	return kvmalloc(n * size + c, gfp);
+}
+#define kvzalloc_ab_c(a, b, c, gfp) kvmalloc_ab_c(a, b, c, (gfp) | __GFP_ZERO)
+
+/**
+ * kvzalloc_struct() - Allocate and zero-fill a structure containing a
+ *		       variable length array.
+ * @p: Pointer to the structure.
+ * @member: Name of the array member.
+ * @n: Number of elements in the array.
+ * @gfp: Memory allocation flags.
+ *
+ * Allocate (and zero-fill) enough memory for a structure with an array
+ * of @n elements.  This function is safe to use when @n is specified by
+ * userspace as the arithmetic will not overflow.
+ * Use kvfree() to free the allocated memory.
+ *
+ * Context: Process context.  May sleep; the @gfp flags should be based on
+ *	    %GFP_KERNEL.
+ * Return: Zero-filled memory or a NULL pointer.
+ */
+#define kvzalloc_struct(p, member, n, gfp)				\
+	(typeof(p))kvzalloc_ab_c(n,					\
+		sizeof(*(p)->member) + __must_be_array((p)->member),	\
+		offsetof(typeof(*(p)), member), gfp)
+
 extern void kvfree(const void *addr);
 
 static inline atomic_t *compound_mapcount_ptr(struct page *page)
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/char/virtio_console.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 468f06134012..e0816cc2c6bd 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -433,8 +433,7 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 	 * Allocate buffer and the sg list. The sg list array is allocated
 	 * directly after the port_buffer struct.
 	 */
-	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
-		      GFP_KERNEL);
+	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
 	if (!buf)
 		goto fail;
 
-- 
2.15.1

[PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/char/virtio_console.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 468f06134012..e0816cc2c6bd 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -433,8 +433,7 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 	 * Allocate buffer and the sg list. The sg list array is allocated
 	 * directly after the port_buffer struct.
 	 */
-	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
-		      GFP_KERNEL);
+	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
 	if (!buf)
 		goto fail;
 
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 4/8] Convert dax device to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/dax/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 2137dbc29877..5821cde340f6 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -586,7 +586,7 @@ struct dev_dax *devm_create_dev_dax(struct dax_region *dax_region,
 	if (!count)
 		return ERR_PTR(-EINVAL);
 
-	dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
+	dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
 	if (!dev_dax)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.15.1

[PATCH v2 4/8] Convert dax device to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/dax/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 2137dbc29877..5821cde340f6 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -586,7 +586,7 @@ struct dev_dax *devm_create_dev_dax(struct dax_region *dax_region,
 	if (!count)
 		return ERR_PTR(-EINVAL);
 
-	dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
+	dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
 	if (!dev_dax)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 5/8] Convert infiniband uverbs to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 4 ++--
 include/rdma/ib_verbs.h              | 5 +----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 256934d1f64f..7e4114a8954d 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3346,8 +3346,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 		goto err_uobj;
 	}
 
-	flow_attr = kzalloc(sizeof(*flow_attr) + cmd.flow_attr.num_of_specs *
-			    sizeof(union ib_flow_spec), GFP_KERNEL);
+	flow_attr = kvzalloc_struct(flow_attr, flows,
+				    cmd.flow_attr.num_of_specs, GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 73b2387e3f74..895509ee7f80 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1977,10 +1977,7 @@ struct ib_flow_attr {
 	u32	     flags;
 	u8	     num_of_specs;
 	u8	     port;
-	/* Following are the optional layers according to user request
-	 * struct ib_flow_spec_xxx
-	 * struct ib_flow_spec_yyy
-	 */
+	union ib_flow_spec flows[];
 };
 
 struct ib_flow {
-- 
2.15.1

[PATCH v2 5/8] Convert infiniband uverbs to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 4 ++--
 include/rdma/ib_verbs.h              | 5 +----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 256934d1f64f..7e4114a8954d 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3346,8 +3346,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 		goto err_uobj;
 	}
 
-	flow_attr = kzalloc(sizeof(*flow_attr) + cmd.flow_attr.num_of_specs *
-			    sizeof(union ib_flow_spec), GFP_KERNEL);
+	flow_attr = kvzalloc_struct(flow_attr, flows,
+				    cmd.flow_attr.num_of_specs, GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 73b2387e3f74..895509ee7f80 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1977,10 +1977,7 @@ struct ib_flow_attr {
 	u32	     flags;
 	u8	     num_of_specs;
 	u8	     port;
-	/* Following are the optional layers according to user request
-	 * struct ib_flow_spec_xxx
-	 * struct ib_flow_spec_yyy
-	 */
+	union ib_flow_spec flows[];
 };
 
 struct ib_flow {
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 6/8] Convert v4l2 event to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/media/v4l2-core/v4l2-event.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index 968c2eb08b5a..20afb01301e8 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -215,8 +215,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	if (elems < 1)
 		elems = 1;
 
-	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
-		       GFP_KERNEL);
+	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
 	if (!sev)
 		return -ENOMEM;
 	for (i = 0; i < elems; i++)
-- 
2.15.1

[PATCH v2 6/8] Convert v4l2 event to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/media/v4l2-core/v4l2-event.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index 968c2eb08b5a..20afb01301e8 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -215,8 +215,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	if (elems < 1)
 		elems = 1;
 
-	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
-		       GFP_KERNEL);
+	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
 	if (!sev)
 		return -ENOMEM;
 	for (i = 0; i < elems; i++)
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 7/8] Convert vhost to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1b3e8d2d5c8b..fa6c8fa80dd1 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1284,7 +1284,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 		return -EOPNOTSUPP;
 	if (mem.nregions > max_mem_regions)
 		return -E2BIG;
-	newmem = kvzalloc(size + mem.nregions * sizeof(*m->regions), GFP_KERNEL);
+	newmem = kvzalloc_struct(newmem, regions, mem.nregions, GFP_KERNEL);
 	if (!newmem)
 		return -ENOMEM;
 
-- 
2.15.1

[PATCH v2 7/8] Convert vhost to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1b3e8d2d5c8b..fa6c8fa80dd1 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1284,7 +1284,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 		return -EOPNOTSUPP;
 	if (mem.nregions > max_mem_regions)
 		return -E2BIG;
-	newmem = kvzalloc(size + mem.nregions * sizeof(*m->regions), GFP_KERNEL);
+	newmem = kvzalloc_struct(newmem, regions, mem.nregions, GFP_KERNEL);
 	if (!newmem)
 		return -ENOMEM;
 
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 8/8] Convert jffs2 acl to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/jffs2/acl.c | 3 ++-
 fs/jffs2/acl.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c
index 7ebacf14837f..9df7feffd6ea 100644
--- a/fs/jffs2/acl.c
+++ b/fs/jffs2/acl.c
@@ -13,6 +13,7 @@
 
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
 #include <linux/time.h>
@@ -133,7 +134,7 @@ static void *jffs2_acl_to_medium(const struct posix_acl *acl, size_t *size)
 	size_t i;
 
 	*size = jffs2_acl_size(acl->a_count);
-	header = kmalloc(sizeof(*header) + acl->a_count * sizeof(*entry), GFP_KERNEL);
+	header = kvzalloc_struct(header, a_entries, acl->a_count, GFP_KERNEL);
 	if (!header)
 		return ERR_PTR(-ENOMEM);
 	header->a_version = cpu_to_je32(JFFS2_ACL_VERSION);
diff --git a/fs/jffs2/acl.h b/fs/jffs2/acl.h
index 2e2b5745c3b7..12d0271bdde3 100644
--- a/fs/jffs2/acl.h
+++ b/fs/jffs2/acl.h
@@ -22,6 +22,7 @@ struct jffs2_acl_entry_short {
 
 struct jffs2_acl_header {
 	jint32_t	a_version;
+	struct jffs2_acl_entry	a_entries[];
 };
 
 #ifdef CONFIG_JFFS2_FS_POSIX_ACL
-- 
2.15.1

[PATCH v2 8/8] Convert jffs2 acl to kvzalloc_struct

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/jffs2/acl.c | 3 ++-
 fs/jffs2/acl.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c
index 7ebacf14837f..9df7feffd6ea 100644
--- a/fs/jffs2/acl.c
+++ b/fs/jffs2/acl.c
@@ -13,6 +13,7 @@
 
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
 #include <linux/time.h>
@@ -133,7 +134,7 @@ static void *jffs2_acl_to_medium(const struct posix_acl *acl, size_t *size)
 	size_t i;
 
 	*size = jffs2_acl_size(acl->a_count);
-	header = kmalloc(sizeof(*header) + acl->a_count * sizeof(*entry), GFP_KERNEL);
+	header = kvzalloc_struct(header, a_entries, acl->a_count, GFP_KERNEL);
 	if (!header)
 		return ERR_PTR(-ENOMEM);
 	header->a_version = cpu_to_je32(JFFS2_ACL_VERSION);
diff --git a/fs/jffs2/acl.h b/fs/jffs2/acl.h
index 2e2b5745c3b7..12d0271bdde3 100644
--- a/fs/jffs2/acl.h
+++ b/fs/jffs2/acl.h
@@ -22,6 +22,7 @@ struct jffs2_acl_entry_short {
 
 struct jffs2_acl_header {
 	jint32_t	a_version;
+	struct jffs2_acl_entry	a_entries[];
 };
 
 #ifdef CONFIG_JFFS2_FS_POSIX_ACL
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  drivers/char/virtio_console.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 468f06134012..e0816cc2c6bd 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -433,8 +433,7 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  	 * Allocate buffer and the sg list. The sg list array is allocated
>  	 * directly after the port_buffer struct.
>  	 */
> -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> -		      GFP_KERNEL);
> +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
>  	if (!buf)
>  		goto fail;

kvfree?

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  drivers/char/virtio_console.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 468f06134012..e0816cc2c6bd 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -433,8 +433,7 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  	 * Allocate buffer and the sg list. The sg list array is allocated
>  	 * directly after the port_buffer struct.
>  	 */
> -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> -		      GFP_KERNEL);
> +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
>  	if (!buf)
>  		goto fail;

kvfree?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 12:19:47PM -0800, Joe Perches wrote:
> On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> >  	 */
> > -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> > -		      GFP_KERNEL);
> > +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
> >  	if (!buf)
> 
> kvfree?

Yes, that would also need to be done.  The point of these last six
patches was to show the API in use, not for applying.

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 12:19:47PM -0800, Joe Perches wrote:
> On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> >  	 */
> > -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> > -		      GFP_KERNEL);
> > +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
> >  	if (!buf)
> 
> kvfree?

Yes, that would also need to be done.  The point of these last six
patches was to show the API in use, not for applying.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:28 -0800, Matthew Wilcox wrote:
> On Wed, Feb 14, 2018 at 12:19:47PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > >  	 */
> > > -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> > > -		      GFP_KERNEL);
> > > +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
> > >  	if (!buf)
> > 
> > kvfree?
> 
> Yes, that would also need to be done.  The point of these last six
> patches was to show the API in use, not for applying.

That's what RFC is for...

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:28 -0800, Matthew Wilcox wrote:
> On Wed, Feb 14, 2018 at 12:19:47PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > >  	 */
> > > -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> > > -		      GFP_KERNEL);
> > > +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
> > >  	if (!buf)
> > 
> > kvfree?
> 
> Yes, that would also need to be done.  The point of these last six
> patches was to show the API in use, not for applying.

That's what RFC is for...

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/8] Convert virtio_console to kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:28 -0800, Matthew Wilcox wrote:
> On Wed, Feb 14, 2018 at 12:19:47PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > >  	 */
> > > -	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
> > > -		      GFP_KERNEL);
> > > +	buf = kvzalloc_struct(buf, sg, pages, GFP_KERNEL);
> > >  	if (!buf)
> > 
> > kvfree?
> 
> Yes, that would also need to be done.  The point of these last six
> patches was to show the API in use, not for applying.

That's what RFC is for...

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> We have kvmalloc_array in order to safely allocate an array with a
> number of elements specified by userspace (avoiding arithmetic overflow
> leading to a buffer overrun).  But it's fairly common to have a header
> in front of that array (eg specifying the length of the array), so we
> need a helper function for that situation.
> 
> kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> of our best efforts to name the arguments, it's really hard to remember
> which order to put the arguments in.  kvzalloc_struct() eliminates that
> effort; you tell it about the struct you're allocating, and it puts the
> arguments in the right order for you (and checks that the arguments
> you've given are at least plausible).
> 
> For comparison between the three schemes:
> 
> 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> 			GFP_KERNEL);
> 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> 			GFP_KERNEL);
> 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);

Perhaps kv[zm]alloc_buf_and_array is better naming.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> We have kvmalloc_array in order to safely allocate an array with a
> number of elements specified by userspace (avoiding arithmetic overflow
> leading to a buffer overrun).  But it's fairly common to have a header
> in front of that array (eg specifying the length of the array), so we
> need a helper function for that situation.
> 
> kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> of our best efforts to name the arguments, it's really hard to remember
> which order to put the arguments in.  kvzalloc_struct() eliminates that
> effort; you tell it about the struct you're allocating, and it puts the
> arguments in the right order for you (and checks that the arguments
> you've given are at least plausible).
> 
> For comparison between the three schemes:
> 
> 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> 			GFP_KERNEL);
> 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> 			GFP_KERNEL);
> 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);

Perhaps kv[zm]alloc_buf_and_array is better naming.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > We have kvmalloc_array in order to safely allocate an array with a
> > number of elements specified by userspace (avoiding arithmetic overflow
> > leading to a buffer overrun).  But it's fairly common to have a header
> > in front of that array (eg specifying the length of the array), so we
> > need a helper function for that situation.
> > 
> > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > of our best efforts to name the arguments, it's really hard to remember
> > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > effort; you tell it about the struct you're allocating, and it puts the
> > arguments in the right order for you (and checks that the arguments
> > you've given are at least plausible).
> > 
> > For comparison between the three schemes:
> > 
> > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > 			GFP_KERNEL);
> > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > 			GFP_KERNEL);
> > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> 
> Perhaps kv[zm]alloc_buf_and_array is better naming.

I think that's actively misleading.  The programmer isn't allocating a
buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
and that made some sense; they're allocating an array with a header.
But nobody thinks about it like that; they're allocating a structure
with a variably sized array at the end of it.

If C macros had decent introspection, I'd like it to be:

	sev = kvzalloc_struct(elems, GFP_KERNEL);

and have the macro examine the structure pointed to by 'sev', check
the last element was an array, calculate the size of the array element,
and call kvzalloc_ab_c.  But we don't live in that world, so I have to
get the programmer to tell me the structure and the name of the last
element in it.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > We have kvmalloc_array in order to safely allocate an array with a
> > number of elements specified by userspace (avoiding arithmetic overflow
> > leading to a buffer overrun).  But it's fairly common to have a header
> > in front of that array (eg specifying the length of the array), so we
> > need a helper function for that situation.
> > 
> > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > of our best efforts to name the arguments, it's really hard to remember
> > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > effort; you tell it about the struct you're allocating, and it puts the
> > arguments in the right order for you (and checks that the arguments
> > you've given are at least plausible).
> > 
> > For comparison between the three schemes:
> > 
> > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > 			GFP_KERNEL);
> > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > 			GFP_KERNEL);
> > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> 
> Perhaps kv[zm]alloc_buf_and_array is better naming.

I think that's actively misleading.  The programmer isn't allocating a
buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
and that made some sense; they're allocating an array with a header.
But nobody thinks about it like that; they're allocating a structure
with a variably sized array at the end of it.

If C macros had decent introspection, I'd like it to be:

	sev = kvzalloc_struct(elems, GFP_KERNEL);

and have the macro examine the structure pointed to by 'sev', check
the last element was an array, calculate the size of the array element,
and call kvzalloc_ab_c.  But we don't live in that world, so I have to
get the programmer to tell me the structure and the name of the last
element in it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote:
> On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > > We have kvmalloc_array in order to safely allocate an array with a
> > > number of elements specified by userspace (avoiding arithmetic overflow
> > > leading to a buffer overrun).  But it's fairly common to have a header
> > > in front of that array (eg specifying the length of the array), so we
> > > need a helper function for that situation.
> > > 
> > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > > of our best efforts to name the arguments, it's really hard to remember
> > > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > > effort; you tell it about the struct you're allocating, and it puts the
> > > arguments in the right order for you (and checks that the arguments
> > > you've given are at least plausible).
> > > 
> > > For comparison between the three schemes:
> > > 
> > > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> > 
> > Perhaps kv[zm]alloc_buf_and_array is better naming.
> 
> I think that's actively misleading.  The programmer isn't allocating a
> buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> and that made some sense; they're allocating an array with a header.
> But nobody thinks about it like that; they're allocating a structure
> with a variably sized array at the end of it.
> 
> If C macros had decent introspection, I'd like it to be:
> 
> 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> 
> and have the macro examine the structure pointed to by 'sev', check
> the last element was an array, calculate the size of the array element,
> and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> get the programmer to tell me the structure and the name of the last
> element in it.

Look at your patch 4

-       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
+       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);

Here what is being allocated is exactly a struct
and an array.

And this doesn't compile either.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote:
> On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > > We have kvmalloc_array in order to safely allocate an array with a
> > > number of elements specified by userspace (avoiding arithmetic overflow
> > > leading to a buffer overrun).  But it's fairly common to have a header
> > > in front of that array (eg specifying the length of the array), so we
> > > need a helper function for that situation.
> > > 
> > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > > of our best efforts to name the arguments, it's really hard to remember
> > > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > > effort; you tell it about the struct you're allocating, and it puts the
> > > arguments in the right order for you (and checks that the arguments
> > > you've given are at least plausible).
> > > 
> > > For comparison between the three schemes:
> > > 
> > > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> > 
> > Perhaps kv[zm]alloc_buf_and_array is better naming.
> 
> I think that's actively misleading.  The programmer isn't allocating a
> buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> and that made some sense; they're allocating an array with a header.
> But nobody thinks about it like that; they're allocating a structure
> with a variably sized array at the end of it.
> 
> If C macros had decent introspection, I'd like it to be:
> 
> 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> 
> and have the macro examine the structure pointed to by 'sev', check
> the last element was an array, calculate the size of the array element,
> and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> get the programmer to tell me the structure and the name of the last
> element in it.

Look at your patch 4

-       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
+       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);

Here what is being allocated is exactly a struct
and an array.

And this doesn't compile either.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote:
> On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > > We have kvmalloc_array in order to safely allocate an array with a
> > > number of elements specified by userspace (avoiding arithmetic overflow
> > > leading to a buffer overrun).  But it's fairly common to have a header
> > > in front of that array (eg specifying the length of the array), so we
> > > need a helper function for that situation.
> > > 
> > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > > of our best efforts to name the arguments, it's really hard to remember
> > > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > > effort; you tell it about the struct you're allocating, and it puts the
> > > arguments in the right order for you (and checks that the arguments
> > > you've given are at least plausible).
> > > 
> > > For comparison between the three schemes:
> > > 
> > > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> > 
> > Perhaps kv[zm]alloc_buf_and_array is better naming.
> 
> I think that's actively misleading.  The programmer isn't allocating a
> buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> and that made some sense; they're allocating an array with a header.
> But nobody thinks about it like that; they're allocating a structure
> with a variably sized array at the end of it.
> 
> If C macros had decent introspection, I'd like it to be:
> 
> 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> 
> and have the macro examine the structure pointed to by 'sev', check
> the last element was an array, calculate the size of the array element,
> and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> get the programmer to tell me the structure and the name of the last
> element in it.

Look at your patch 4

-       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
+       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);

Here what is being allocated is exactly a struct
and an array.

And this doesn't compile either.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 13:24 -0800, Joe Perches wrote:
> Look at your patch 4
> 
> -       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
> +       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
> 
> Here what is being allocated is exactly a struct
> and an array.
> 
> And this doesn't compile either.

apologies,  my mistake.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 13:24 -0800, Joe Perches wrote:
> Look at your patch 4
> 
> -       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
> +       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
> 
> Here what is being allocated is exactly a struct
> and an array.
> 
> And this doesn't compile either.

apologies,  my mistake.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Joe Perches]

On Wed, 2018-02-14 at 13:24 -0800, Joe Perches wrote:
> Look at your patch 4
> 
> -       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
> +       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
> 
> Here what is being allocated is exactly a struct
> and an array.
> 
> And this doesn't compile either.

apologies,  my mistake.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 01:24:09PM -0800, Joe Perches wrote:
> On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote:
> > On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > > Perhaps kv[zm]alloc_buf_and_array is better naming.
> > 
> > I think that's actively misleading.  The programmer isn't allocating a
> > buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> > and that made some sense; they're allocating an array with a header.
> > But nobody thinks about it like that; they're allocating a structure
> > with a variably sized array at the end of it.
> > 
> > If C macros had decent introspection, I'd like it to be:
> > 
> > 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> > 
> > and have the macro examine the structure pointed to by 'sev', check
> > the last element was an array, calculate the size of the array element,
> > and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> > get the programmer to tell me the structure and the name of the last
> > element in it.
> 
> Look at your patch 4
> 
> -       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
> +       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
> 
> Here what is being allocated is exactly a struct
> and an array.

No, it's a struct *containing* an array.  Look at patches 5 & 8 where I
have to convert the structs to contain the array which was silently
being allocated immediately after them.

> And this doesn't compile either.

Does for me.  What error are you seeing?

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 01:24:09PM -0800, Joe Perches wrote:
> On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote:
> > On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > > Perhaps kv[zm]alloc_buf_and_array is better naming.
> > 
> > I think that's actively misleading.  The programmer isn't allocating a
> > buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> > and that made some sense; they're allocating an array with a header.
> > But nobody thinks about it like that; they're allocating a structure
> > with a variably sized array at the end of it.
> > 
> > If C macros had decent introspection, I'd like it to be:
> > 
> > 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> > 
> > and have the macro examine the structure pointed to by 'sev', check
> > the last element was an array, calculate the size of the array element,
> > and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> > get the programmer to tell me the structure and the name of the last
> > element in it.
> 
> Look at your patch 4
> 
> -       dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
> +       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
> 
> Here what is being allocated is exactly a struct
> and an array.

No, it's a struct *containing* an array.  Look at patches 5 & 8 where I
have to convert the structs to contain the array which was silently
being allocated immediately after them.

> And this doesn't compile either.

Does for me.  What error are you seeing?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Andrew Morton]

On Wed, 14 Feb 2018 13:12:03 -0800 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > > We have kvmalloc_array in order to safely allocate an array with a
> > > number of elements specified by userspace (avoiding arithmetic overflow
> > > leading to a buffer overrun).  But it's fairly common to have a header
> > > in front of that array (eg specifying the length of the array), so we
> > > need a helper function for that situation.
> > > 
> > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > > of our best efforts to name the arguments, it's really hard to remember
> > > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > > effort; you tell it about the struct you're allocating, and it puts the
> > > arguments in the right order for you (and checks that the arguments
> > > you've given are at least plausible).
> > > 
> > > For comparison between the three schemes:
> > > 
> > > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> > 
> > Perhaps kv[zm]alloc_buf_and_array is better naming.
> 
> I think that's actively misleading.  The programmer isn't allocating a
> buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> and that made some sense; they're allocating an array with a header.
> But nobody thinks about it like that; they're allocating a structure
> with a variably sized array at the end of it.
> 
> If C macros had decent introspection, I'd like it to be:
> 
> 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> 
> and have the macro examine the structure pointed to by 'sev', check
> the last element was an array, calculate the size of the array element,
> and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> get the programmer to tell me the structure and the name of the last
> element in it.

hm, bikeshedding fun.


struct foo {
	whatevs;
	struct bar[0];
}


	struct foo *a_foo;

	a_foo = kvzalloc_struct_buf(foo, bar, nr_bars);

and macro magic will insert the `struct' keyword.  This will help to
force a miscompile if inappropriate types are used for foo and bar.

Problem is, foo may be a union(?) and bar may be a scalar type.  So

	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);

or, of course.

	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
				    nr_bars);

or whatever.

The basic idea is to use the wrapper macros to force compile errors if
these things are misused.


Also,

> +/**
> + * kvmalloc_ab_c() - Allocate (a * b + c) bytes of memory.
> + * @n: Number of elements.
> + * @size: Size of each element (should be constant).
> + * @c: Size of header (should be constant).
> + * @gfp: Memory allocation flags.
> + *
> + * Use this function to allocate @n * @size + @c bytes of memory.  This
> + * function is safe to use when @n is controlled from userspace; it will
> + * return %NULL if the required amount of memory cannot be allocated.
> + * Use kvfree() to free the allocated memory.
> + *
> + * The kvzalloc_struct() function is easier to use as it has typechecking
> + * and you do not need to remember which of the arguments should be constants.
> + *
> + * Context: Process context.  May sleep; the @gfp flags should be based on
> + *	    %GFP_KERNEL.
> + * Return: A pointer to the allocated memory or %NULL.
> + */
> +static inline __must_check
> +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> +{
> +	if (size != 0 && n > (SIZE_MAX - c) / size)
> +		return NULL;
> +
> +	return kvmalloc(n * size + c, gfp);
> +}

Can we please avoid the single-char identifiers?

void *kvmalloc_ab_c(size_t n_elems, size_t elem_size, size_t header_size,
		    gfp_t gfp);

makes the code so much more readable.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Andrew Morton]

On Wed, 14 Feb 2018 13:12:03 -0800 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote:
> > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote:
> > > We have kvmalloc_array in order to safely allocate an array with a
> > > number of elements specified by userspace (avoiding arithmetic overflow
> > > leading to a buffer overrun).  But it's fairly common to have a header
> > > in front of that array (eg specifying the length of the array), so we
> > > need a helper function for that situation.
> > > 
> > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > > of our best efforts to name the arguments, it's really hard to remember
> > > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > > effort; you tell it about the struct you're allocating, and it puts the
> > > arguments in the right order for you (and checks that the arguments
> > > you've given are at least plausible).
> > > 
> > > For comparison between the three schemes:
> > > 
> > > 	sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > > 			GFP_KERNEL);
> > > 	sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> > 
> > Perhaps kv[zm]alloc_buf_and_array is better naming.
> 
> I think that's actively misleading.  The programmer isn't allocating a
> buf, they're allocating a struct.  kvzalloc_hdr_arr was the earlier name,
> and that made some sense; they're allocating an array with a header.
> But nobody thinks about it like that; they're allocating a structure
> with a variably sized array at the end of it.
> 
> If C macros had decent introspection, I'd like it to be:
> 
> 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> 
> and have the macro examine the structure pointed to by 'sev', check
> the last element was an array, calculate the size of the array element,
> and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> get the programmer to tell me the structure and the name of the last
> element in it.

hm, bikeshedding fun.


struct foo {
	whatevs;
	struct bar[0];
}


	struct foo *a_foo;

	a_foo = kvzalloc_struct_buf(foo, bar, nr_bars);

and macro magic will insert the `struct' keyword.  This will help to
force a miscompile if inappropriate types are used for foo and bar.

Problem is, foo may be a union(?) and bar may be a scalar type.  So

	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);

or, of course.

	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
				    nr_bars);

or whatever.

The basic idea is to use the wrapper macros to force compile errors if
these things are misused.


Also,

> +/**
> + * kvmalloc_ab_c() - Allocate (a * b + c) bytes of memory.
> + * @n: Number of elements.
> + * @size: Size of each element (should be constant).
> + * @c: Size of header (should be constant).
> + * @gfp: Memory allocation flags.
> + *
> + * Use this function to allocate @n * @size + @c bytes of memory.  This
> + * function is safe to use when @n is controlled from userspace; it will
> + * return %NULL if the required amount of memory cannot be allocated.
> + * Use kvfree() to free the allocated memory.
> + *
> + * The kvzalloc_struct() function is easier to use as it has typechecking
> + * and you do not need to remember which of the arguments should be constants.
> + *
> + * Context: Process context.  May sleep; the @gfp flags should be based on
> + *	    %GFP_KERNEL.
> + * Return: A pointer to the allocated memory or %NULL.
> + */
> +static inline __must_check
> +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> +{
> +	if (size != 0 && n > (SIZE_MAX - c) / size)
> +		return NULL;
> +
> +	return kvmalloc(n * size + c, gfp);
> +}

Can we please avoid the single-char identifiers?

void *kvmalloc_ab_c(size_t n_elems, size_t elem_size, size_t header_size,
		    gfp_t gfp);

makes the code so much more readable.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 03:58:33PM -0800, Andrew Morton wrote:
> On Wed, 14 Feb 2018 13:12:03 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> > If C macros had decent introspection, I'd like it to be:
> > 
> > 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> > 
> > and have the macro examine the structure pointed to by 'sev', check
> > the last element was an array, calculate the size of the array element,
> > and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> > get the programmer to tell me the structure and the name of the last
> > element in it.
> 
> hm, bikeshedding fun.

Heck, yeah!  Fun!

> struct foo {
> 	whatevs;
> 	struct bar[0];
> }
> 
> 
> 	struct foo *a_foo;
> 
> 	a_foo = kvzalloc_struct_buf(foo, bar, nr_bars);
> 
> and macro magic will insert the `struct' keyword.  This will help to
> force a miscompile if inappropriate types are used for foo and bar.
> 
> Problem is, foo may be a union(?) and bar may be a scalar type.  So
> 
> 	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);
> 
> or, of course.
> 
> 	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
> 				    nr_bars);
> 
> or whatever.

I think that's actually *less* checking than the option I presented here.
My version has the compiler check:
1. You assigned the pointer to the allocated memory
2. ... to a pointer of compatible type with p
3. p is a pointer
4. member is a member of the type p points to
5. member is an array type

Your version doesn't check point 4, and it'd be easy to get it wrong like this:

struct quux {
	int n;
	struct foo foos[];
} *p = kvmalloc_struct(struct quux, struct foo *, n);

or vice-versa:

struct quux {
	int n;
	struct foo *foos[];
} *p = kvmalloc_struct(struct quux, struct foo, n);

What is it that you don't like about my version?  Is it passing the
uninitialised pointer to a macro that looks like a function?  Because
we do this all the time:

	struct foo *p = kmalloc(sizeof(*p), GFP_KERNEL);

> The basic idea is to use the wrapper macros to force compile errors if
> these things are misused.

Right.  Although passing the pointer in lets us work this magic on an
unnamed struct.  Like this mess...

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 7874c980d569..5cd3e127bea8 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -792,7 +792,7 @@ static void uncore_type_exit(struct intel_uncore_type *type)
 		kfree(type->pmus);
 		type->pmus = NULL;
 	}
-	kfree(type->events_group);
+	kvfree(type->events_group);
 	type->events_group = NULL;
 }
 
@@ -805,8 +805,6 @@ static void uncore_types_exit(struct intel_uncore_type **types)
 static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 {
 	struct intel_uncore_pmu *pmus;
-	struct attribute_group *attr_group;
-	struct attribute **attrs;
 	size_t size;
 	int i, j;
 
@@ -831,21 +829,24 @@ static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 				0, type->num_counters, 0, 0);
 
 	if (type->event_descs) {
+		struct {
+			struct attribute_group group;
+			struct attribute *attrs[];
+		} *attr_group;
 		for (i = 0; type->event_descs[i].attr.attr.name; i++);
 
-		attr_group = kzalloc(sizeof(struct attribute *) * (i + 1) +
-					sizeof(*attr_group), GFP_KERNEL);
+		attr_group = kvzalloc_struct(attr_group, attrs, i + 1,
+								GFP_KERNEL);
 		if (!attr_group)
 			goto err;
 
-		attrs = (struct attribute **)(attr_group + 1);
-		attr_group->name = "events";
-		attr_group->attrs = attrs;
+		attr_group->group.name = "events";
+		attr_group->group.attrs = attr_group->attrs;
 
 		for (j = 0; j < i; j++)
-			attrs[j] = &type->event_descs[j].attr.attr;
+			attr_group->attrs[j] = &type->event_descs[j].attr.attr;
 
-		type->events_group = attr_group;
+		type->events_group = &attr_group->group;
 	}
 
 	type->pmu_group = &uncore_pmu_attr_group;

> > +static inline __must_check
> > +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> > +{
> > +	if (size != 0 && n > (SIZE_MAX - c) / size)
> > +		return NULL;
> > +
> > +	return kvmalloc(n * size + c, gfp);
> > +}
> 
> Can we please avoid the single-char identifiers?
> 
> void *kvmalloc_ab_c(size_t n_elems, size_t elem_size, size_t header_size,
> 		    gfp_t gfp);
> 
> makes the code so much more readable.

I was naming for consistency:

static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
{
        if (size != 0 && n > SIZE_MAX / size)
                return NULL;

I'll happily change 'c' to 'hdr_size' though.

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 03:58:33PM -0800, Andrew Morton wrote:
> On Wed, 14 Feb 2018 13:12:03 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> > If C macros had decent introspection, I'd like it to be:
> > 
> > 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> > 
> > and have the macro examine the structure pointed to by 'sev', check
> > the last element was an array, calculate the size of the array element,
> > and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> > get the programmer to tell me the structure and the name of the last
> > element in it.
> 
> hm, bikeshedding fun.

Heck, yeah!  Fun!

> struct foo {
> 	whatevs;
> 	struct bar[0];
> }
> 
> 
> 	struct foo *a_foo;
> 
> 	a_foo = kvzalloc_struct_buf(foo, bar, nr_bars);
> 
> and macro magic will insert the `struct' keyword.  This will help to
> force a miscompile if inappropriate types are used for foo and bar.
> 
> Problem is, foo may be a union(?) and bar may be a scalar type.  So
> 
> 	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);
> 
> or, of course.
> 
> 	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
> 				    nr_bars);
> 
> or whatever.

I think that's actually *less* checking than the option I presented here.
My version has the compiler check:
1. You assigned the pointer to the allocated memory
2. ... to a pointer of compatible type with p
3. p is a pointer
4. member is a member of the type p points to
5. member is an array type

Your version doesn't check point 4, and it'd be easy to get it wrong like this:

struct quux {
	int n;
	struct foo foos[];
} *p = kvmalloc_struct(struct quux, struct foo *, n);

or vice-versa:

struct quux {
	int n;
	struct foo *foos[];
} *p = kvmalloc_struct(struct quux, struct foo, n);

What is it that you don't like about my version?  Is it passing the
uninitialised pointer to a macro that looks like a function?  Because
we do this all the time:

	struct foo *p = kmalloc(sizeof(*p), GFP_KERNEL);

> The basic idea is to use the wrapper macros to force compile errors if
> these things are misused.

Right.  Although passing the pointer in lets us work this magic on an
unnamed struct.  Like this mess...

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 7874c980d569..5cd3e127bea8 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -792,7 +792,7 @@ static void uncore_type_exit(struct intel_uncore_type *type)
 		kfree(type->pmus);
 		type->pmus = NULL;
 	}
-	kfree(type->events_group);
+	kvfree(type->events_group);
 	type->events_group = NULL;
 }
 
@@ -805,8 +805,6 @@ static void uncore_types_exit(struct intel_uncore_type **types)
 static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 {
 	struct intel_uncore_pmu *pmus;
-	struct attribute_group *attr_group;
-	struct attribute **attrs;
 	size_t size;
 	int i, j;
 
@@ -831,21 +829,24 @@ static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 				0, type->num_counters, 0, 0);
 
 	if (type->event_descs) {
+		struct {
+			struct attribute_group group;
+			struct attribute *attrs[];
+		} *attr_group;
 		for (i = 0; type->event_descs[i].attr.attr.name; i++);
 
-		attr_group = kzalloc(sizeof(struct attribute *) * (i + 1) +
-					sizeof(*attr_group), GFP_KERNEL);
+		attr_group = kvzalloc_struct(attr_group, attrs, i + 1,
+								GFP_KERNEL);
 		if (!attr_group)
 			goto err;
 
-		attrs = (struct attribute **)(attr_group + 1);
-		attr_group->name = "events";
-		attr_group->attrs = attrs;
+		attr_group->group.name = "events";
+		attr_group->group.attrs = attr_group->attrs;
 
 		for (j = 0; j < i; j++)
-			attrs[j] = &type->event_descs[j].attr.attr;
+			attr_group->attrs[j] = &type->event_descs[j].attr.attr;
 
-		type->events_group = attr_group;
+		type->events_group = &attr_group->group;
 	}
 
 	type->pmu_group = &uncore_pmu_attr_group;

> > +static inline __must_check
> > +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> > +{
> > +	if (size != 0 && n > (SIZE_MAX - c) / size)
> > +		return NULL;
> > +
> > +	return kvmalloc(n * size + c, gfp);
> > +}
> 
> Can we please avoid the single-char identifiers?
> 
> void *kvmalloc_ab_c(size_t n_elems, size_t elem_size, size_t header_size,
> 		    gfp_t gfp);
> 
> makes the code so much more readable.

I was naming for consistency:

static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
{
        if (size != 0 && n > SIZE_MAX / size)
                return NULL;

I'll happily change 'c' to 'hdr_size' though.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 07:40:50PM -0800, Matthew Wilcox wrote:
> > 	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);
> > 
> > or, of course.
> > 
> > 	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
> > 				    nr_bars);
> > 
> > or whatever.

This version works, although it's more typing in the callers:

-               attr_group = kvzalloc_struct(attr_group, attrs, i + 1,
+               attr_group = kvzalloc_struct(typeof(*attr_group), attrs, i + 1,
                                                                GFP_KERNEL);
...
-       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
+       dev_dax = kvzalloc_struct(struct dev_dax, res, count, GFP_KERNEL);
...
-#define kvzalloc_struct(p, member, n, gfp)                             \
-       (typeof(p))kvzalloc_ab_c(n,                                     \
-               sizeof(*(p)->member) + __must_be_array((p)->member),    \
-               offsetof(typeof(*(p)), member), gfp)
+#define kvzalloc_struct(s, member, n, gfp) ({                          \
+       s *__p;                                                         \
+       (s *)kvzalloc_ab_c(n,                                           \
+               sizeof(*(__p)->member) + __must_be_array((__p)->member),\
+               offsetof(s, member), gfp);                              \
+})

Gives all the same checking as the current version, and doesn't involve
passing an uninitialised pointer to the macro.

It also looks pretty similar:

	p = kvzalloc(sizeof(*p), GFP_KERNEL);
	p = kvzalloc_struct(typeof(*p), array, count, GFP_KERNEL);
	p = kvzalloc_array(sizeof(*p), count);

Re: [PATCH v2 2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 07:40:50PM -0800, Matthew Wilcox wrote:
> > 	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);
> > 
> > or, of course.
> > 
> > 	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
> > 				    nr_bars);
> > 
> > or whatever.

This version works, although it's more typing in the callers:

-               attr_group = kvzalloc_struct(attr_group, attrs, i + 1,
+               attr_group = kvzalloc_struct(typeof(*attr_group), attrs, i + 1,
                                                                GFP_KERNEL);
...
-       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
+       dev_dax = kvzalloc_struct(struct dev_dax, res, count, GFP_KERNEL);
...
-#define kvzalloc_struct(p, member, n, gfp)                             \
-       (typeof(p))kvzalloc_ab_c(n,                                     \
-               sizeof(*(p)->member) + __must_be_array((p)->member),    \
-               offsetof(typeof(*(p)), member), gfp)
+#define kvzalloc_struct(s, member, n, gfp) ({                          \
+       s *__p;                                                         \
+       (s *)kvzalloc_ab_c(n,                                           \
+               sizeof(*(__p)->member) + __must_be_array((__p)->member),\
+               offsetof(s, member), gfp);                              \
+})

Gives all the same checking as the current version, and doesn't involve
passing an uninitialised pointer to the macro.

It also looks pretty similar:

	p = kvzalloc(sizeof(*p), GFP_KERNEL);
	p = kvzalloc_struct(typeof(*p), array, count, GFP_KERNEL);
	p = kvzalloc_array(sizeof(*p), count);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>