Re: [PATCH v7 2/3] net/9p/usbg: Add new usb gadget function transport

[Oliver Neukum <oneukum@suse.com>]

On 22.07.24 00:08, Michael Grzeschik wrote:

> +
> +static int usb9pfs_queue_tx(struct f_usb9pfs *usb9pfs, struct usb_request *req,
> +			    gfp_t gfp_flags)
> +{
> +	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> +	int ret = -ENOMEM;

No need. This will be overwritten.

> +
> +	if (!(usb9pfs->p9_tx_req->tc.size % usb9pfs->in_ep->maxpacket))
> +		req->zero = 1;
> +
> +	req->buf = usb9pfs->p9_tx_req->tc.sdata;
> +	req->length = usb9pfs->p9_tx_req->tc.size;
> +
> +	dev_dbg(&cdev->gadget->dev, "%s usb9pfs send --> %d/%d, zero: %d\n",
> +		usb9pfs->in_ep->name, req->actual, req->length, req->zero);
> +
> +	ret = usb_ep_queue(usb9pfs->in_ep, req, gfp_flags);
> +
> +	dev_dbg(&cdev->gadget->dev, "tx submit --> %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int usb9pfs_queue_rx(struct f_usb9pfs *usb9pfs, struct usb_request *req,
> +			    gfp_t gfp_flags)
> +{
> +	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> +	int ret = -ENOMEM;

Overwritten in literally the next statement.

> +	ret = usb_ep_queue(usb9pfs->out_ep, req, gfp_flags);
> +
> +	dev_dbg(&cdev->gadget->dev, "rx submit --> %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int usb9pfs_transmit(struct f_usb9pfs *usb9pfs)
> +{
> +	struct p9_req_t *p9_req = NULL;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&usb9pfs->lock, flags);
> +	if (usb9pfs->p9_tx_req) {
> +		spin_unlock_irqrestore(&usb9pfs->lock, flags);
> +		return -EBUSY;
> +	}
> +
> +	p9_req = list_first_entry_or_null(&usb9pfs->tx_req_list,
> +					  struct p9_req_t, req_list);
> +	if (!p9_req) {
> +		spin_unlock_irqrestore(&usb9pfs->lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	list_del(&p9_req->req_list);

You have deleted it from the list

> +	usb9pfs->p9_tx_req = p9_req;
> +
> +	p9_req_get(usb9pfs->p9_tx_req);
> +
> +	ret = usb9pfs_queue_tx(usb9pfs, usb9pfs->in_req, GFP_ATOMIC);

This means that if this function returns an error, the deletion
from the list may or may not have happened.

> +	spin_unlock_irqrestore(&usb9pfs->lock, flags);
> +
> +	return ret;
> +}
> +
> +static void usb9pfs_tx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_usb9pfs *usb9pfs = ep->driver_data;
> +	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> +	int ret = 0;
> +
> +	if (req->status) {
> +		dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
> +			ep->name, req->status, req->actual, req->length);
> +		return;
> +	}
> +
> +	/* reset zero packages */
> +	req->zero = 0;
> +
> +	dev_dbg(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
> +		ep->name, req->status, req->actual, req->length);
> +
> +	WRITE_ONCE(usb9pfs->p9_tx_req->status, REQ_STATUS_SENT);
> +
> +	p9_req_put(usb9pfs->client, usb9pfs->p9_tx_req);
> +
> +	ret = usb9pfs_queue_rx(usb9pfs, usb9pfs->out_req, GFP_ATOMIC);
> +	if (ret)
> +		return;

Ehhh ? Could you explain the error handling here?

> +
> +	return;
> +}
> +
> +static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf)
> +{
> +	struct p9_req_t *p9_rx_req;
> +	struct p9_fcall	rc;
> +	int ret;
> +
> +	/* start by reading header */
> +	rc.sdata = buf;
> +	rc.offset = 0;
> +	rc.capacity = P9_HDRSZ;
> +	rc.size = P9_HDRSZ;
> +
> +	p9_debug(P9_DEBUG_TRANS, "mux %p got %zu bytes\n", usb9pfs,
> +		 rc.capacity - rc.offset);
> +
> +	ret = p9_parse_header(&rc, &rc.size, NULL, NULL, 0);
> +	if (ret) {
> +		p9_debug(P9_DEBUG_ERROR,
> +			 "error parsing header: %d\n", ret);
> +		return NULL;
> +	}
> +
> +	p9_debug(P9_DEBUG_TRANS,
> +		 "mux %p pkt: size: %d bytes tag: %d\n",
> +		 usb9pfs, rc.size, rc.tag);
> +
> +	p9_rx_req = p9_tag_lookup(usb9pfs->client, rc.tag);
> +	if (!p9_rx_req || p9_rx_req->status != REQ_STATUS_SENT) {
> +		p9_debug(P9_DEBUG_ERROR, "Unexpected packet tag %d\n", rc.tag);
> +		return NULL;
> +	}
> +
> +	if (rc.size > p9_rx_req->rc.capacity) {
> +		p9_debug(P9_DEBUG_ERROR,
> +			 "requested packet size too big: %d for tag %d with capacity %zd\n",
> +			 rc.size, rc.tag, p9_rx_req->rc.capacity);
> +		p9_req_put(usb9pfs->client, p9_rx_req);
> +		return NULL;
> +	}
> +
> +	if (!p9_rx_req->rc.sdata) {
> +		p9_debug(P9_DEBUG_ERROR,
> +			 "No recv fcall for tag %d (req %p), disconnecting!\n",
> +			 rc.tag, p9_rx_req);
> +		p9_req_put(usb9pfs->client, p9_rx_req);
> +		return NULL;
> +	}
> +
> +	return p9_rx_req;
> +}
> +
> +static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_usb9pfs *usb9pfs = ep->driver_data;
> +	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
> +	struct p9_req_t *p9_rx_req;
> +	unsigned long flags;
> +
> +	if (req->status) {
> +		dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
> +			ep->name, req->status, req->actual, req->length);
> +		return;
> +	}
> +
> +	p9_rx_req = usb9pfs_rx_header(usb9pfs, req->buf);
> +	if (!p9_rx_req)
> +		return;
> +
> +	memcpy(p9_rx_req->rc.sdata, req->buf, req->actual);
> +
> +	p9_rx_req->rc.size = req->actual;
> +
> +	p9_client_cb(usb9pfs->client, p9_rx_req, REQ_STATUS_RCVD);
> +	p9_req_put(usb9pfs->client, p9_rx_req);
> +
> +	spin_lock_irqsave(&usb9pfs->lock, flags);
> +	usb9pfs->p9_tx_req = NULL;
> +
> +	spin_unlock_irqrestore(&usb9pfs->lock, flags);

Why can usb9pfs_tx_complete() touch this without taking the spinlock?

> +
> +	usb9pfs_transmit(usb9pfs);

This can fail. What happens then?

> +
> +	return;
> +}
> +


[..]

> +static int p9_usbg_cancel(struct p9_client *client, struct p9_req_t *req)

This ought to be boolean

> +{
> +	struct f_usb9pfs *usb9pfs = client->trans;
> +	unsigned long flags;
> +	int ret = 1;
> +
> +	p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req);
> +
> +	spin_lock_irqsave(&usb9pfs->lock, flags);
> +
> +	if (req->status == REQ_STATUS_UNSENT) {
> +		list_del(&req->req_list);
> +		WRITE_ONCE(req->status, REQ_STATUS_FLSHD);
> +		p9_req_put(client, req);
> +		ret = 0;
> +	}
> +	spin_unlock_irqrestore(&usb9pfs->lock, flags);
> +
> +	return ret;
> +}

	Regards
		Oliver