diff mbox series

usb: gadget: Don't allocate zero-length buffers

Message ID 20190320125641.10690-1-andrzej.p@collabora.com (mailing list archive)
State New, archived
Headers show
Series usb: gadget: Don't allocate zero-length buffers | expand

Commit Message

Andrzej Pietrasiewicz March 20, 2019, 12:56 p.m. UTC 
Don't actually allocate anything if userspace enqueues a zero-length
buffer. Otherwise vmalloc of zero-sized area will be attempted in
ffs_build_sg_list().

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/usb/gadget/function/f_fs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Greg KH March 20, 2019, 2:25 p.m. UTC | #1 
On Wed, Mar 20, 2019 at 01:56:41PM +0100, Andrzej Pietrasiewicz wrote:
> Don't actually allocate anything if userspace enqueues a zero-length
> buffer. Otherwise vmalloc of zero-sized area will be attempted in
> ffs_build_sg_list().
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 08357c5dd026..5cecfe3e9395 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -797,6 +797,9 @@ static void *ffs_build_sg_list(struct sg_table *sgt, size_t sz)
>  static inline void *ffs_alloc_buffer(struct ffs_io_data *io_data,
>  	size_t data_len)
>  {
> +	if (!data_len)
> +		return ZERO_SIZE_PTR;
> +
>  	if (io_data->use_sg)
>  		return ffs_build_sg_list(&io_data->sgt, data_len);
>  
> @@ -805,7 +808,7 @@ static inline void *ffs_alloc_buffer(struct ffs_io_data *io_data,
>  
>  static inline void ffs_free_buffer(struct ffs_io_data *io_data)
>  {
> -	if (!io_data->buf)
> +	if (ZERO_OR_NULL_PTR(io_data->buf))
>  		return;
>  
>  	if (io_data->use_sg) {

Are you sure this is ok?  zero length packets mean something in the USB
protocol :)

What is the problem of trying to allocate a buffer of 0 bytes?

thanks,

greg k-h
Andrzej Pietrasiewicz March 21, 2019, 7:42 a.m. UTC | #2 
Hi Greg,

W dniu 20.03.2019 o 15:25, Greg KH pisze:
> On Wed, Mar 20, 2019 at 01:56:41PM +0100, Andrzej Pietrasiewicz wrote:
>> Don't actually allocate anything if userspace enqueues a zero-length
>> buffer. Otherwise vmalloc of zero-sized area will be attempted in
>> ffs_build_sg_list().
>>
<snip>


>> Are you sure this is ok?  zero length packets mean something in the USB
>> protocol :)


Yeah, I do know they do!


>> What is the problem of trying to allocate a buffer of 0 bytes?
>>
My understanding is that if a zero-length buffer is enqueued, then

nobody will read from it (the read will be zero bytes, to be precise),

so returning ZERO_SIZE_PTR shouldn't hurt. On the other hand,

if scatter-gather mode is used (depends on what the underlying UDC declares)

then ffs_alloc_buffer() takes the ffs_build_sg_list() branch and then 
vmalloc does

the below:

[   23.637611] some-process: vmalloc: allocation failure: 0 bytes, 
mode:0x6000c0(GFP_KERNEL), nodemask=
(null)
[   23.642435] CPU: 3 PID: 307 Comm: some-process Not tainted 
5.0.0-00048-g762eb03d5d27-dirty #151
[   23.650903] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   23.656983] [<c0112464>] (unwind_backtrace) from [<c010df90>] 
(show_stack+0x10/0x14)
[   23.664705] [<c010df90>] (show_stack) from [<c0a53914>] 
(dump_stack+0x98/0xc4)
[   23.671906] [<c0a53914>] (dump_stack) from [<c023b18c>] 
(warn_alloc+0xd0/0x168)
[   23.679111] [<c023b18c>] (warn_alloc) from [<c027921c>] 
(__vmalloc_node_range+0x230/0x25c)
[   23.687350] [<c027921c>] (__vmalloc_node_range) from [<c02794e4>] 
(__vmalloc_node.constprop.12+0x48/0x
50)
[   23.696904] [<c02794e4>] (__vmalloc_node.constprop.12) from 
[<c0279580>] (vmalloc+0x2c/0x34)
[   23.705328] [<c0279580>] (vmalloc) from [<c06a76cc>] 
(ffs_epfile_io+0x4ac/0x750)
[   23.712693] [<c06a76cc>] (ffs_epfile_io) from [<c06a7a24>] 
(ffs_epfile_write_iter+0xb4/0x168)
[   23.721209] [<c06a7a24>] (ffs_epfile_write_iter) from [<c0295004>] 
(__vfs_write+0x118/0x17c)
[   23.729623] [<c0295004>] (__vfs_write) from [<c02951f0>] 
(vfs_write+0xa4/0x184)
[   23.736914] [<c02951f0>] (vfs_write) from [<c0295410>] 
(ksys_write+0x4c/0xac)
[   23.744025] [<c0295410>] (ksys_write) from [<c0101000>] 
(ret_fast_syscall+0x0/0x28)
[   23.751664] Exception stack(0xec8e1fa8 to 0xec8e1ff0)
[   23.756686] 1fa0:                   00000000 b6b02cc0 00000004 
b57fed53 00000000 00000004
[   23.764861] 1fc0: 00000000 b6b02cc0 00000000 00000004 b6b02cc0 
00000000 00000000 00000000
[   23.773019] 1fe0: 00000000 b57fed40 00000000 b6de73d0
[   23.779588] Mem-Info:
[   23.780342] active_anon:1530 inactive_anon:4412 isolated_anon:0
[   23.780342]  active_file:3680 inactive_file:3342 isolated_file:0
[   23.780342]  unevictable:0 dirty:18 writeback:0 unstable:0
[   23.780342]  slab_reclaimable:4245 slab_unreclaimable:4430
[   23.780342]  mapped:2496 shmem:4427 pagetables:79 bounce:0
[   23.780342]  free:478387 free_pcp:1242 free_cma:24244
[   23.813879] Node 0 active_anon:6120kB inactive_anon:17648kB 
active_file:14720kB inactive_file:13368kB
unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:9984kB 
dirty:72kB writeback:0kB shmem:17708k
B writeback_tmp:0kB unstable:0kB all_unreclaimable? no
[   23.836882] Normal free:696520kB min:3452kB low:4312kB high:5172kB 
active_anon:0kB inactive_anon:0kB a
ctive_file:7576kB inactive_file:1028kB unevictable:0kB writepending:52kB 
present:786432kB managed:746456k
B mlocked:0kB kernel_stack:824kB pagetables:0kB bounce:0kB 
free_pcp:2624kB local_pcp:508kB free_cma:0kB
[   23.864320] lowmem_reserve[]: 0 9112 9112
[   23.868056] HighMem free:1217028kB min:512kB low:1860kB high:3208kB 
active_anon:6120kB inactive_anon:1
7648kB active_file:7144kB inactive_file:12340kB unevictable:0kB 
writepending:20kB present:1264640kB manag
ed:1264640kB mlocked:0kB kernel_stack:0kB pagetables:316kB bounce:0kB 
free_pcp:2336kB local_pcp:800kB fre
e_cma:96976kB
[   23.896900] lowmem_reserve[]: 0 0 0
[   23.900161] Normal: 2*4kB (UM) 23*8kB (UME) 9*16kB (UM) 0*32kB 1*64kB 
(M) 2*128kB (UM) 2*256kB (ME) 2*
512kB (UE) 0*1024kB 5*2048kB (UME) 167*4096kB (M) = 696464kB
[   23.914775] HighMem: 1*4kB (U) 0*8kB 2*16kB (UC) 1*32kB (U) 1*64kB 
(C) 3*128kB (UMC) 0*256kB 2*512kB (
UC) 1*1024kB (M) 3*2048kB (UMC) 295*4096kB (MC) = 1217028kB
[   23.929179] 11488 total pagecache pages
[   23.932933] 0 pages in swap cache
[   23.936179] Swap cache stats: add 0, delete 0, find 0/0
[   23.941439] Free swap  = 0kB
[   23.944246] Total swap = 0kB
[   23.947112] 512768 pages RAM
[   23.949974] 316160 pages HighMem/MovableOnly
[   23.954282] 9994 pages reserved
[   23.957357] 24576 pages cma reserved
Michał Nazarewicz March 29, 2019, 7:54 p.m. UTC | #3 
Sorry for the late reply.

On Thu, 21 Mar 2019 at 07:42, Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Hi Greg,
>
> W dniu 20.03.2019 o 15:25, Greg KH pisze:
> > On Wed, Mar 20, 2019 at 01:56:41PM +0100, Andrzej Pietrasiewicz wrote:
> >> Don't actually allocate anything if userspace enqueues a zero-length
> >> buffer. Otherwise vmalloc of zero-sized area will be attempted in
> >> ffs_build_sg_list().

> >>
> <snip>
>
>
> >> Are you sure this is ok?  zero length packets mean something in the USB
> >> protocol :)
>
>
> Yeah, I do know they do!
>
>
> >> What is the problem of trying to allocate a buffer of 0 bytes?
> >>
> My understanding is that if a zero-length buffer is enqueued, then
> nobody will read from it (the read will be zero bytes, to be precise),
> so returning ZERO_SIZE_PTR shouldn't hurt. On the other hand,
> if scatter-gather mode is used (depends on what the underlying UDC declares)
> then ffs_alloc_buffer() takes the ffs_build_sg_list() branch and then
> vmalloc does the below:

I thought sg is not used if size of the buffer is no more than a page:
https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_fs.c#L1000


> [   23.637611] some-process: vmalloc: allocation failure: 0 bytes,
> mode:0x6000c0(GFP_KERNEL), nodemask=
> (null)
> [   23.642435] CPU: 3 PID: 307 Comm: some-process Not tainted
> 5.0.0-00048-g762eb03d5d27-dirty #151
> [   23.650903] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   23.656983] [<c0112464>] (unwind_backtrace) from [<c010df90>]
> (show_stack+0x10/0x14)
> [   23.664705] [<c010df90>] (show_stack) from [<c0a53914>]
> (dump_stack+0x98/0xc4)
> [   23.671906] [<c0a53914>] (dump_stack) from [<c023b18c>]
> (warn_alloc+0xd0/0x168)
> [   23.679111] [<c023b18c>] (warn_alloc) from [<c027921c>]
> (__vmalloc_node_range+0x230/0x25c)
> [   23.687350] [<c027921c>] (__vmalloc_node_range) from [<c02794e4>]
> (__vmalloc_node.constprop.12+0x48/0x
> 50)
> [   23.696904] [<c02794e4>] (__vmalloc_node.constprop.12) from
> [<c0279580>] (vmalloc+0x2c/0x34)
> [   23.705328] [<c0279580>] (vmalloc) from [<c06a76cc>]
> (ffs_epfile_io+0x4ac/0x750)
> [   23.712693] [<c06a76cc>] (ffs_epfile_io) from [<c06a7a24>]
> (ffs_epfile_write_iter+0xb4/0x168)
> [   23.721209] [<c06a7a24>] (ffs_epfile_write_iter) from [<c0295004>]
> (__vfs_write+0x118/0x17c)
> [   23.729623] [<c0295004>] (__vfs_write) from [<c02951f0>]
> (vfs_write+0xa4/0x184)
> [   23.736914] [<c02951f0>] (vfs_write) from [<c0295410>]
> (ksys_write+0x4c/0xac)
> [   23.744025] [<c0295410>] (ksys_write) from [<c0101000>]
> (ret_fast_syscall+0x0/0x28)
> [   23.751664] Exception stack(0xec8e1fa8 to 0xec8e1ff0)
> [   23.756686] 1fa0:                   00000000 b6b02cc0 00000004
> b57fed53 00000000 00000004
> [   23.764861] 1fc0: 00000000 b6b02cc0 00000000 00000004 b6b02cc0
> 00000000 00000000 00000000
> [   23.773019] 1fe0: 00000000 b57fed40 00000000 b6de73d0
> [   23.779588] Mem-Info:
> [   23.780342] active_anon:1530 inactive_anon:4412 isolated_anon:0
> [   23.780342]  active_file:3680 inactive_file:3342 isolated_file:0
> [   23.780342]  unevictable:0 dirty:18 writeback:0 unstable:0
> [   23.780342]  slab_reclaimable:4245 slab_unreclaimable:4430
> [   23.780342]  mapped:2496 shmem:4427 pagetables:79 bounce:0
> [   23.780342]  free:478387 free_pcp:1242 free_cma:24244
> [   23.813879] Node 0 active_anon:6120kB inactive_anon:17648kB
> active_file:14720kB inactive_file:13368kB
> unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:9984kB
> dirty:72kB writeback:0kB shmem:17708k
> B writeback_tmp:0kB unstable:0kB all_unreclaimable? no
> [   23.836882] Normal free:696520kB min:3452kB low:4312kB high:5172kB
> active_anon:0kB inactive_anon:0kB a
> ctive_file:7576kB inactive_file:1028kB unevictable:0kB writepending:52kB
> present:786432kB managed:746456k
> B mlocked:0kB kernel_stack:824kB pagetables:0kB bounce:0kB
> free_pcp:2624kB local_pcp:508kB free_cma:0kB
> [   23.864320] lowmem_reserve[]: 0 9112 9112
> [   23.868056] HighMem free:1217028kB min:512kB low:1860kB high:3208kB
> active_anon:6120kB inactive_anon:1
> 7648kB active_file:7144kB inactive_file:12340kB unevictable:0kB
> writepending:20kB present:1264640kB manag
> ed:1264640kB mlocked:0kB kernel_stack:0kB pagetables:316kB bounce:0kB
> free_pcp:2336kB local_pcp:800kB fre
> e_cma:96976kB
> [   23.896900] lowmem_reserve[]: 0 0 0
> [   23.900161] Normal: 2*4kB (UM) 23*8kB (UME) 9*16kB (UM) 0*32kB 1*64kB
> (M) 2*128kB (UM) 2*256kB (ME) 2*
> 512kB (UE) 0*1024kB 5*2048kB (UME) 167*4096kB (M) = 696464kB
> [   23.914775] HighMem: 1*4kB (U) 0*8kB 2*16kB (UC) 1*32kB (U) 1*64kB
> (C) 3*128kB (UMC) 0*256kB 2*512kB (
> UC) 1*1024kB (M) 3*2048kB (UMC) 295*4096kB (MC) = 1217028kB
> [   23.929179] 11488 total pagecache pages
> [   23.932933] 0 pages in swap cache
> [   23.936179] Swap cache stats: add 0, delete 0, find 0/0
> [   23.941439] Free swap  = 0kB
> [   23.944246] Total swap = 0kB
> [   23.947112] 512768 pages RAM
> [   23.949974] 316160 pages HighMem/MovableOnly
> [   23.954282] 9994 pages reserved
> [   23.957357] 24576 pages cma reserved
>
>
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 08357c5dd026..5cecfe3e9395 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -797,6 +797,9 @@  static void *ffs_build_sg_list(struct sg_table *sgt, size_t sz)
 static inline void *ffs_alloc_buffer(struct ffs_io_data *io_data,
 	size_t data_len)
 {
+	if (!data_len)
+		return ZERO_SIZE_PTR;
+
 	if (io_data->use_sg)
 		return ffs_build_sg_list(&io_data->sgt, data_len);
 
@@ -805,7 +808,7 @@  static inline void *ffs_alloc_buffer(struct ffs_io_data *io_data,
 
 static inline void ffs_free_buffer(struct ffs_io_data *io_data)
 {
-	if (!io_data->buf)
+	if (ZERO_OR_NULL_PTR(io_data->buf))
 		return;
 
 	if (io_data->use_sg) {