Message-ID: <0804469c-664a-219d-bb6a-b4e5f133edd9@i-love.sakura.ne.jp>
Date:   Sun, 19 Dec 2021 16:59:44 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH] usb: fixing some clang warnings inside usb host drivers
Content-Language: en-US
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Julio Faracco <jcfaracco@gmail.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        axboe@kernel.dk, tglx@linutronix.de, damien.lemoal@wdc.com,
        dkadashev@gmail.com, paul.gortmaker@windriver.com,
        zhouyanjie@wanyeetech.com, niklas.cassel@wdc.com,
        macro@orcam.me.uk, caihuoqing@baidu.com
References: <20211218042420.28466-1-jcfaracco@gmail.com>
 <Yb4i7LyYIlJi/9fb@rowland.harvard.edu>
 <7c5bbc97-b9dc-96bb-5764-58bebec0178d@i-love.sakura.ne.jp>
 <Yb6d7tflQeJ+1Et2@rowland.harvard.edu>
From:   Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
In-Reply-To: <Yb6d7tflQeJ+1Et2@rowland.harvard.edu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021/12/19 11:50, Alan Stern wrote:
> You should read this code in usb_submit_urb():
> 
> 	max = usb_endpoint_maxp(&ep->desc);
> 	if (max <= 0) {
> 		dev_dbg(&dev->dev,
> 			"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
> 			usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
> 			__func__, max);
> 		return -EMSGSIZE;
> 	}
> 
> As far as I know, every code path leading to qtd_fill() has to pass this 
> test.

Excuse me, but surely qtd_fill() is using the result from usb_maxpacket()

----------------------------------------
static struct list_head *
qh_urb_transaction (
	struct ehci_hcd		*ehci,
	struct urb		*urb,
	struct list_head	*head,
	gfp_t			flags
) {
(...snipped...)
	maxpacket = usb_maxpacket(urb->dev, urb->pipe, !is_input);

	/*
	 * buffer gets wrapped in one or more qtds;
	 * last one may be "short" (including zero len)
	 * and may serve as a control status ack
	 */
	for (;;) {
		int this_qtd_len;

		this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
				maxpacket);
		this_sg_len -= this_qtd_len;
		len -= this_qtd_len;
		buf += this_qtd_len;
(...snipped...)
}
----------------------------------------

and usb_maxpacket() may return 0 ?

----------------------------------------
static inline __u16
usb_maxpacket(struct usb_device *udev, int pipe, int is_out)
{
	struct usb_host_endpoint	*ep;
	unsigned			epnum = usb_pipeendpoint(pipe);

	if (is_out) {
		WARN_ON(usb_pipein(pipe));
		ep = udev->ep_out[epnum];
	} else {
		WARN_ON(usb_pipeout(pipe));
		ep = udev->ep_in[epnum];
	}
	if (!ep)
		return 0;

	/* NOTE:  only 0x07ff bits are for packet size... */
	return usb_endpoint_maxp(&ep->desc);
}
----------------------------------------

If we don't need to care about the possibility of returning 0 (including
all possible race conditions taken into account), please explain it as a
comment block.