[PATCH] block: bio: use struct_size() in kmalloc()

[PATCH] block: bio: use struct_size() in kmalloc()

[xiaolinkui]

One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:

struct foo {
    int stuff;
    struct boo entry[];
};

instance = kmalloc(sizeof(struct foo) + count * sizeof(struct boo), GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:

instance = kmalloc(struct_size(instance, entry, count), GFP_KERNEL);

Signed-off-by: xiaolinkui <xiaolinkui@kylinos.cn>
---
 block/bio.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 683cbb4..847ac60 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -436,9 +436,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
 		if (nr_iovecs > UIO_MAXIOV)
 			return NULL;
 
-		p = kmalloc(sizeof(struct bio) +
-			    nr_iovecs * sizeof(struct bio_vec),
-			    gfp_mask);
+		p = kmalloc(struct_size(bio, bi_io_vec, nr_iovecs), gfp_mask);
 		front_pad = 0;
 		inline_vecs = nr_iovecs;
 	} else {
@@ -1120,8 +1118,7 @@ static struct bio_map_data *bio_alloc_map_data(struct iov_iter *data,
 	if (data->nr_segs > UIO_MAXIOV)
 		return NULL;
 
-	bmd = kmalloc(sizeof(struct bio_map_data) +
-		       sizeof(struct iovec) * data->nr_segs, gfp_mask);
+	bmd = kmalloc(struct_size(bmd, iov, data->nr_segs), gfp_mask);
 	if (!bmd)
 		return NULL;
 	memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);
-- 
2.7.4

Re: [PATCH] block: bio: use struct_size() in kmalloc()

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 5/17/19 2:17 AM, xiaolinkui wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct foo {
>     int stuff;
>     struct boo entry[];
> };
>
> instance = kmalloc(sizeof(struct foo) + count * sizeof(struct boo), GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> instance = kmalloc(struct_size(instance, entry, count), GFP_KERNEL);
>
> Signed-off-by: xiaolinkui <xiaolinkui@kylinos.cn>
> ---
>  block/bio.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 683cbb4..847ac60 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -436,9 +436,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
>  		if (nr_iovecs > UIO_MAXIOV)
>  			return NULL;
>  
> -		p = kmalloc(sizeof(struct bio) +
> -			    nr_iovecs * sizeof(struct bio_vec),
> -			    gfp_mask);
> +		p = kmalloc(struct_size(bio, bi_io_vec, nr_iovecs), gfp_mask);
>  		front_pad = 0;
>  		inline_vecs = nr_iovecs;
>  	} else {
> @@ -1120,8 +1118,7 @@ static struct bio_map_data *bio_alloc_map_data(struct iov_iter *data,
>  	if (data->nr_segs > UIO_MAXIOV)
>  		return NULL;
>  
> -	bmd = kmalloc(sizeof(struct bio_map_data) +
> -		       sizeof(struct iovec) * data->nr_segs, gfp_mask);
> +	bmd = kmalloc(struct_size(bmd, iov, data->nr_segs), gfp_mask);
>  	if (!bmd)
>  		return NULL;
>  	memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);

Re: [PATCH] block: bio: use struct_size() in kmalloc()

[Jens Axboe]

On 5/17/19 3:12 AM, xiaolinkui wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
> 
> struct foo {
>     int stuff;
>     struct boo entry[];
> };
> 
> instance = kmalloc(sizeof(struct foo) + count * sizeof(struct boo), GFP_KERNEL);
> 
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
> 
> instance = kmalloc(struct_size(instance, entry, count), GFP_KERNEL);

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] block: bio: use struct_size() in kmalloc()

[Jens Axboe]

On 5/17/19 3:17 PM, Jens Axboe wrote:
> On 5/17/19 3:12 AM, xiaolinkui wrote:
>> One of the more common cases of allocation size calculations is finding
>> the size of a structure that has a zero-sized array at the end, along
>> with memory for some number of elements for that array. For example:
>>
>> struct foo {
>>     int stuff;
>>     struct boo entry[];
>> };
>>
>> instance = kmalloc(sizeof(struct foo) + count * sizeof(struct boo), GFP_KERNEL);
>>
>> Instead of leaving these open-coded and prone to type mistakes, we can
>> now use the new struct_size() helper:
>>
>> instance = kmalloc(struct_size(instance, entry, count), GFP_KERNEL);
> 
> Applied, thanks.

I take that back, you obviously didn't even compile this patch. Never
send untested crap, without explicitly saying so.

-- 
Jens Axboe

Re: [PATCH] block: bio: use struct_size() in kmalloc()

[Chaitanya Kulkarni]

- linux-block@vger.kernel.org <linux-block@vger.kernel.org> to reduce
the noise.

I apologies Jens, I didn't apply and tested these patches before
submitting the
review and assumed that patches are compiled and tested, I'll do so for each
patch before submitting the review.

Xiaolinkui,

Please send compiled and tested patch only on the latest kernel on the
appropriate subsystem, otherwise mark the patch appropriately
[RFC/Compile only] so reviewer would know without such a tag
it is easy to assume that patch is compiled and tested.

You have also sent out the couple of more patches with this fix.

If they are not compiled and tested with right kernel branch for each
subsystem, please update the appropriate mail thread either to ignore those
patches (if they have compilation problem on appropriate branch) or mark
them compile test only (this needs to be avoided for these patches), in
either
case please send updated patches for this fix if needed.

Thanks.

On 5/17/19 3:59 PM, Jens Axboe wrote:
> On 5/17/19 3:17 PM, Jens Axboe wrote:
>> On 5/17/19 3:12 AM, xiaolinkui wrote:
>>> One of the more common cases of allocation size calculations is finding
>>> the size of a structure that has a zero-sized array at the end, along
>>> with memory for some number of elements for that array. For example:
>>>
>>> struct foo {
>>>     int stuff;
>>>     struct boo entry[];
>>> };
>>>
>>> instance = kmalloc(sizeof(struct foo) + count * sizeof(struct boo), GFP_KERNEL);
>>>
>>> Instead of leaving these open-coded and prone to type mistakes, we can
>>> now use the new struct_size() helper:
>>>
>>> instance = kmalloc(struct_size(instance, entry, count), GFP_KERNEL);
>> Applied, thanks.
> I take that back, you obviously didn't even compile this patch. Never
> send untested crap, without explicitly saying so.
>

Re: [PATCH] block: bio: use struct_size() in kmalloc()

[Jens Axboe]

On 5/17/19 6:43 PM, Chaitanya Kulkarni wrote:
> - linux-block@vger.kernel.org <linux-block@vger.kernel.org> to reduce
> the noise.
> 
> I apologies Jens, I didn't apply and tested these patches before
> submitting the review and assumed that patches are compiled and
> tested, I'll do so for each patch before submitting the review.

Just to be clear, I'm not placing any blame on you. It's easy to miss
that kind of thing in a review. The onus is on the submitter to ensure
that anything he/she sends in has been both compile and runtime tested.

> Xiaolinkui,
> 
> Please send compiled and tested patch only on the latest kernel on the
> appropriate subsystem, otherwise mark the patch appropriately
> [RFC/Compile only] so reviewer would know without such a tag
> it is easy to assume that patch is compiled and tested.
> 
> You have also sent out the couple of more patches with this fix.
> 
> If they are not compiled and tested with right kernel branch for each
> subsystem, please update the appropriate mail thread either to ignore those
> patches (if they have compilation problem on appropriate branch) or mark
> them compile test only (this needs to be avoided for these patches), in
> either
> case please send updated patches for this fix if needed.

This is solid advice. Sending out untested patches without EXPLICITLY
saying so is reckless and irresponsible, and causes harm to your
reputation as well. Trust is an important part of being successful in an
open source project.

-- 
Jens Axboe

Re: [PATCH] block: bio: Use struct_size() in kmalloc()

[Jens Axboe]

On 6/10/19 9:04 AM, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
> 
> struct bio_map_data {
> 	...
>          struct iovec iov[];
> };
> 
> instance = kmalloc(sizeof(sizeof(struct bio_map_data) + sizeof(struct iovec) *
>                            count, GFP_KERNEL);
> 
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
> 
> instance = kmalloc(struct_size(instance, iov, count), GFP_KERNEL);
> 
> This code was detected with the help of Coccinelle.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] block: bio: Use struct_size() in kmalloc()

[Kees Cook]

On Mon, Jun 10, 2019 at 10:04:12AM -0500, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
> 
> struct bio_map_data {
> 	...
>         struct iovec iov[];
> };
> 
> instance = kmalloc(sizeof(sizeof(struct bio_map_data) + sizeof(struct iovec) *
>                           count, GFP_KERNEL);
> 
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
> 
> instance = kmalloc(struct_size(instance, iov, count), GFP_KERNEL);
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  block/bio.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 683cbb40f051..4bcdcd3f63f4 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1120,8 +1120,7 @@ static struct bio_map_data *bio_alloc_map_data(struct iov_iter *data,
>  	if (data->nr_segs > UIO_MAXIOV)
>  		return NULL;
>  
> -	bmd = kmalloc(sizeof(struct bio_map_data) +
> -		       sizeof(struct iovec) * data->nr_segs, gfp_mask);
> +	bmd = kmalloc(struct_size(bmd, iov, data->nr_segs), gfp_mask);
>  	if (!bmd)
>  		return NULL;
>  	memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);
> -- 
> 2.21.0
> 

-- 
Kees Cook

[PATCH] block: bio: Use struct_size() in kmalloc()

[Gustavo A. R. Silva]

One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:

struct bio_map_data {
	...
        struct iovec iov[];
};

instance = kmalloc(sizeof(sizeof(struct bio_map_data) + sizeof(struct iovec) *
                          count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:

instance = kmalloc(struct_size(instance, iov, count), GFP_KERNEL);

This code was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 block/bio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 683cbb40f051..4bcdcd3f63f4 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1120,8 +1120,7 @@ static struct bio_map_data *bio_alloc_map_data(struct iov_iter *data,
 	if (data->nr_segs > UIO_MAXIOV)
 		return NULL;
 
-	bmd = kmalloc(sizeof(struct bio_map_data) +
-		       sizeof(struct iovec) * data->nr_segs, gfp_mask);
+	bmd = kmalloc(struct_size(bmd, iov, data->nr_segs), gfp_mask);
 	if (!bmd)
 		return NULL;
 	memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);
-- 
2.21.0