diff mbox series

usb: gadget: eem: Use common error handling code in eem_unwrap()

Message ID 59fadd5a-6574-4379-98ac-cc4f11b675cc@web.de
State New
Headers show
Series usb: gadget: eem: Use common error handling code in eem_unwrap() | expand

Commit Message

Markus Elfring Sept. 25, 2024, 3:38 p.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Sep 2024 17:26:40 +0200

Add jump targets so that a bit of exception handling can be better reused
at the end of this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/usb/gadget/function/f_eem.c | 30 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 16 deletions(-)

--
2.46.1

Comments

kernel test robot Sept. 26, 2024, 5:27 a.m. UTC | #1 
Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.11 next-20240925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/usb-gadget-eem-Use-common-error-handling-code-in-eem_unwrap/20240925-233931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/59fadd5a-6574-4379-98ac-cc4f11b675cc%40web.de
patch subject: [PATCH] usb: gadget: eem: Use common error handling code in eem_unwrap()
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240926/202409261347.SAA10KGy-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409261347.SAA10KGy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409261347.SAA10KGy-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/usb/gadget/function/f_eem.c: In function 'eem_unwrap':
>> drivers/usb/gadget/function/f_eem.c:547:29: error: 'ep' undeclared (first use in this function); did you mean 'up'?
     547 |         usb_ep_free_request(ep, req);
         |                             ^~
         |                             up
   drivers/usb/gadget/function/f_eem.c:547:29: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/usb/gadget/function/f_eem.c:547:33: error: 'req' undeclared (first use in this function); did you mean 'rq'?
     547 |         usb_ep_free_request(ep, req);
         |                                 ^~~
         |                                 rq
>> drivers/usb/gadget/function/f_eem.c:549:27: error: 'skb2' undeclared (first use in this function); did you mean 'skb'?
     549 |         dev_kfree_skb_any(skb2);
         |                           ^~~~
         |                           skb


vim +547 drivers/usb/gadget/function/f_eem.c

   387	
   388	/*
   389	 * Remove the EEM header.  Note that there can be many EEM packets in a single
   390	 * USB transfer, so we need to break them out and handle them independently.
   391	 */
   392	static int eem_unwrap(struct gether *port,
   393				struct sk_buff *skb,
   394				struct sk_buff_head *list)
   395	{
   396		struct usb_composite_dev	*cdev = port->func.config->cdev;
   397		int				status = 0;
   398	
   399		do {
   400			struct sk_buff	*skb2;
   401			u16		header;
   402			u16		len = 0;
   403	
   404			if (skb->len < EEM_HLEN) {
   405				status = -EINVAL;
   406				DBG(cdev, "invalid EEM header\n");
   407				goto error;
   408			}
   409	
   410			/* remove the EEM header */
   411			header = get_unaligned_le16(skb->data);
   412			skb_pull(skb, EEM_HLEN);
   413	
   414			/* EEM packet header format:
   415			 * b0..14:	EEM type dependent (data or command)
   416			 * b15:		bmType (0 == data, 1 == command)
   417			 */
   418			if (header & BIT(15)) {
   419				struct usb_request	*req;
   420				struct in_context	*ctx;
   421				struct usb_ep		*ep;
   422				u16			bmEEMCmd;
   423	
   424				/* EEM command packet format:
   425				 * b0..10:	bmEEMCmdParam
   426				 * b11..13:	bmEEMCmd
   427				 * b14:		reserved (must be zero)
   428				 * b15:		bmType (1 == command)
   429				 */
   430				if (header & BIT(14))
   431					continue;
   432	
   433				bmEEMCmd = (header >> 11) & 0x7;
   434				switch (bmEEMCmd) {
   435				case 0: /* echo */
   436					len = header & 0x7FF;
   437					if (skb->len < len) {
   438						status = -EOVERFLOW;
   439						goto error;
   440					}
   441	
   442					skb2 = skb_clone(skb, GFP_ATOMIC);
   443					if (unlikely(!skb2)) {
   444						DBG(cdev, "EEM echo response error\n");
   445						goto next;
   446					}
   447					skb_trim(skb2, len);
   448					put_unaligned_le16(BIT(15) | BIT(11) | len,
   449								skb_push(skb2, 2));
   450	
   451					ep = port->in_ep;
   452					req = usb_ep_alloc_request(ep, GFP_ATOMIC);
   453					if (!req)
   454						goto free_skb;
   455	
   456					req->buf = kmalloc(skb2->len, GFP_KERNEL);
   457					if (!req->buf)
   458						goto free_request;
   459	
   460					ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
   461					if (!ctx) {
   462						kfree(req->buf);
   463						goto free_request;
   464					}
   465					ctx->skb = skb2;
   466					ctx->ep = ep;
   467	
   468					skb_copy_bits(skb2, 0, req->buf, skb2->len);
   469					req->length = skb2->len;
   470					req->complete = eem_cmd_complete;
   471					req->zero = 1;
   472					req->context = ctx;
   473					if (usb_ep_queue(port->in_ep, req, GFP_ATOMIC))
   474						DBG(cdev, "echo response queue fail\n");
   475					break;
   476	
   477				case 1:  /* echo response */
   478				case 2:  /* suspend hint */
   479				case 3:  /* response hint */
   480				case 4:  /* response complete hint */
   481				case 5:  /* tickle */
   482				default: /* reserved */
   483					continue;
   484				}
   485			} else {
   486				u32		crc, crc2;
   487				struct sk_buff	*skb3;
   488	
   489				/* check for zero-length EEM packet */
   490				if (header == 0)
   491					continue;
   492	
   493				/* EEM data packet format:
   494				 * b0..13:	length of ethernet frame
   495				 * b14:		bmCRC (0 == sentinel, 1 == calculated)
   496				 * b15:		bmType (0 == data)
   497				 */
   498				len = header & 0x3FFF;
   499				if ((skb->len < len)
   500						|| (len < (ETH_HLEN + ETH_FCS_LEN))) {
   501					status = -EINVAL;
   502					goto error;
   503				}
   504	
   505				/* validate CRC */
   506				if (header & BIT(14)) {
   507					crc = get_unaligned_le32(skb->data + len
   508								- ETH_FCS_LEN);
   509					crc2 = ~crc32_le(~0,
   510							skb->data, len - ETH_FCS_LEN);
   511				} else {
   512					crc = get_unaligned_be32(skb->data + len
   513								- ETH_FCS_LEN);
   514					crc2 = 0xdeadbeef;
   515				}
   516				if (crc != crc2) {
   517					DBG(cdev, "invalid EEM CRC\n");
   518					goto next;
   519				}
   520	
   521				skb2 = skb_clone(skb, GFP_ATOMIC);
   522				if (unlikely(!skb2)) {
   523					DBG(cdev, "unable to unframe EEM packet\n");
   524					goto next;
   525				}
   526				skb_trim(skb2, len - ETH_FCS_LEN);
   527	
   528				skb3 = skb_copy_expand(skb2,
   529							NET_IP_ALIGN,
   530							0,
   531							GFP_ATOMIC);
   532				if (unlikely(!skb3))
   533					goto free_skb;
   534	
   535				dev_kfree_skb_any(skb2);
   536				skb_queue_tail(list, skb3);
   537			}
   538	next:
   539			skb_pull(skb, len);
   540		} while (skb->len);
   541	
   542	error:
   543		dev_kfree_skb_any(skb);
   544		return status;
   545	
   546	free_request:
 > 547		usb_ep_free_request(ep, req);
   548	free_skb:
 > 549		dev_kfree_skb_any(skb2);
   550		goto next;
   551	}
   552
kernel test robot Sept. 26, 2024, 9:03 a.m. UTC | #2 
Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/usb-gadget-eem-Use-common-error-handling-code-in-eem_unwrap/20240925-233931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/59fadd5a-6574-4379-98ac-cc4f11b675cc%40web.de
patch subject: [PATCH] usb: gadget: eem: Use common error handling code in eem_unwrap()
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240926/202409261628.MdRLCYzn-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 7773243d9916f98ba0ffce0c3a960e4aa9f03e81)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409261628.MdRLCYzn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409261628.MdRLCYzn-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/usb/gadget/function/f_eem.c:13:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:10:
   In file included from include/linux/mm.h:2232:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   In file included from drivers/usb/gadget/function/f_eem.c:13:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/usb/gadget/function/f_eem.c:13:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/usb/gadget/function/f_eem.c:13:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/usb/gadget/function/f_eem.c:547:22: error: use of undeclared identifier 'ep'
     547 |         usb_ep_free_request(ep, req);
         |                             ^
>> drivers/usb/gadget/function/f_eem.c:547:26: error: use of undeclared identifier 'req'
     547 |         usb_ep_free_request(ep, req);
         |                                 ^
>> drivers/usb/gadget/function/f_eem.c:549:20: error: use of undeclared identifier 'skb2'; did you mean 'skb'?
     549 |         dev_kfree_skb_any(skb2);
         |                           ^~~~
         |                           skb
   drivers/usb/gadget/function/f_eem.c:393:20: note: 'skb' declared here
     393 |                         struct sk_buff *skb,
         |                                         ^
   7 warnings and 3 errors generated.


vim +/ep +547 drivers/usb/gadget/function/f_eem.c

   387	
   388	/*
   389	 * Remove the EEM header.  Note that there can be many EEM packets in a single
   390	 * USB transfer, so we need to break them out and handle them independently.
   391	 */
   392	static int eem_unwrap(struct gether *port,
   393				struct sk_buff *skb,
   394				struct sk_buff_head *list)
   395	{
   396		struct usb_composite_dev	*cdev = port->func.config->cdev;
   397		int				status = 0;
   398	
   399		do {
   400			struct sk_buff	*skb2;
   401			u16		header;
   402			u16		len = 0;
   403	
   404			if (skb->len < EEM_HLEN) {
   405				status = -EINVAL;
   406				DBG(cdev, "invalid EEM header\n");
   407				goto error;
   408			}
   409	
   410			/* remove the EEM header */
   411			header = get_unaligned_le16(skb->data);
   412			skb_pull(skb, EEM_HLEN);
   413	
   414			/* EEM packet header format:
   415			 * b0..14:	EEM type dependent (data or command)
   416			 * b15:		bmType (0 == data, 1 == command)
   417			 */
   418			if (header & BIT(15)) {
   419				struct usb_request	*req;
   420				struct in_context	*ctx;
   421				struct usb_ep		*ep;
   422				u16			bmEEMCmd;
   423	
   424				/* EEM command packet format:
   425				 * b0..10:	bmEEMCmdParam
   426				 * b11..13:	bmEEMCmd
   427				 * b14:		reserved (must be zero)
   428				 * b15:		bmType (1 == command)
   429				 */
   430				if (header & BIT(14))
   431					continue;
   432	
   433				bmEEMCmd = (header >> 11) & 0x7;
   434				switch (bmEEMCmd) {
   435				case 0: /* echo */
   436					len = header & 0x7FF;
   437					if (skb->len < len) {
   438						status = -EOVERFLOW;
   439						goto error;
   440					}
   441	
   442					skb2 = skb_clone(skb, GFP_ATOMIC);
   443					if (unlikely(!skb2)) {
   444						DBG(cdev, "EEM echo response error\n");
   445						goto next;
   446					}
   447					skb_trim(skb2, len);
   448					put_unaligned_le16(BIT(15) | BIT(11) | len,
   449								skb_push(skb2, 2));
   450	
   451					ep = port->in_ep;
   452					req = usb_ep_alloc_request(ep, GFP_ATOMIC);
   453					if (!req)
   454						goto free_skb;
   455	
   456					req->buf = kmalloc(skb2->len, GFP_KERNEL);
   457					if (!req->buf)
   458						goto free_request;
   459	
   460					ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
   461					if (!ctx) {
   462						kfree(req->buf);
   463						goto free_request;
   464					}
   465					ctx->skb = skb2;
   466					ctx->ep = ep;
   467	
   468					skb_copy_bits(skb2, 0, req->buf, skb2->len);
   469					req->length = skb2->len;
   470					req->complete = eem_cmd_complete;
   471					req->zero = 1;
   472					req->context = ctx;
   473					if (usb_ep_queue(port->in_ep, req, GFP_ATOMIC))
   474						DBG(cdev, "echo response queue fail\n");
   475					break;
   476	
   477				case 1:  /* echo response */
   478				case 2:  /* suspend hint */
   479				case 3:  /* response hint */
   480				case 4:  /* response complete hint */
   481				case 5:  /* tickle */
   482				default: /* reserved */
   483					continue;
   484				}
   485			} else {
   486				u32		crc, crc2;
   487				struct sk_buff	*skb3;
   488	
   489				/* check for zero-length EEM packet */
   490				if (header == 0)
   491					continue;
   492	
   493				/* EEM data packet format:
   494				 * b0..13:	length of ethernet frame
   495				 * b14:		bmCRC (0 == sentinel, 1 == calculated)
   496				 * b15:		bmType (0 == data)
   497				 */
   498				len = header & 0x3FFF;
   499				if ((skb->len < len)
   500						|| (len < (ETH_HLEN + ETH_FCS_LEN))) {
   501					status = -EINVAL;
   502					goto error;
   503				}
   504	
   505				/* validate CRC */
   506				if (header & BIT(14)) {
   507					crc = get_unaligned_le32(skb->data + len
   508								- ETH_FCS_LEN);
   509					crc2 = ~crc32_le(~0,
   510							skb->data, len - ETH_FCS_LEN);
   511				} else {
   512					crc = get_unaligned_be32(skb->data + len
   513								- ETH_FCS_LEN);
   514					crc2 = 0xdeadbeef;
   515				}
   516				if (crc != crc2) {
   517					DBG(cdev, "invalid EEM CRC\n");
   518					goto next;
   519				}
   520	
   521				skb2 = skb_clone(skb, GFP_ATOMIC);
   522				if (unlikely(!skb2)) {
   523					DBG(cdev, "unable to unframe EEM packet\n");
   524					goto next;
   525				}
   526				skb_trim(skb2, len - ETH_FCS_LEN);
   527	
   528				skb3 = skb_copy_expand(skb2,
   529							NET_IP_ALIGN,
   530							0,
   531							GFP_ATOMIC);
   532				if (unlikely(!skb3))
   533					goto free_skb;
   534	
   535				dev_kfree_skb_any(skb2);
   536				skb_queue_tail(list, skb3);
   537			}
   538	next:
   539			skb_pull(skb, len);
   540		} while (skb->len);
   541	
   542	error:
   543		dev_kfree_skb_any(skb);
   544		return status;
   545	
   546	free_request:
 > 547		usb_ep_free_request(ep, req);
   548	free_skb:
 > 549		dev_kfree_skb_any(skb2);
   550		goto next;
   551	}
   552
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
index 6de81ea17274..b1be23e947dc 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -450,24 +450,17 @@  static int eem_unwrap(struct gether *port,

 				ep = port->in_ep;
 				req = usb_ep_alloc_request(ep, GFP_ATOMIC);
-				if (!req) {
-					dev_kfree_skb_any(skb2);
-					goto next;
-				}
+				if (!req)
+					goto free_skb;

 				req->buf = kmalloc(skb2->len, GFP_KERNEL);
-				if (!req->buf) {
-					usb_ep_free_request(ep, req);
-					dev_kfree_skb_any(skb2);
-					goto next;
-				}
+				if (!req->buf)
+					goto free_request;

 				ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
 				if (!ctx) {
 					kfree(req->buf);
-					usb_ep_free_request(ep, req);
-					dev_kfree_skb_any(skb2);
-					goto next;
+					goto free_request;
 				}
 				ctx->skb = skb2;
 				ctx->ep = ep;
@@ -536,10 +529,9 @@  static int eem_unwrap(struct gether *port,
 						NET_IP_ALIGN,
 						0,
 						GFP_ATOMIC);
-			if (unlikely(!skb3)) {
-				dev_kfree_skb_any(skb2);
-				goto next;
-			}
+			if (unlikely(!skb3))
+				goto free_skb;
+
 			dev_kfree_skb_any(skb2);
 			skb_queue_tail(list, skb3);
 		}
@@ -550,6 +542,12 @@  static int eem_unwrap(struct gether *port,
 error:
 	dev_kfree_skb_any(skb);
 	return status;
+
+free_request:
+	usb_ep_free_request(ep, req);
+free_skb:
+	dev_kfree_skb_any(skb2);
+	goto next;
 }

 static inline struct f_eem_opts *to_f_eem_opts(struct config_item *item)