| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zsz4UKIOf5li0bb0@pengutronix.de>
Date: Mon, 26 Aug 2024 23:49:04 +0200
From: Michael Grzeschik <mgr@...gutronix.de>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: andrzej.p@...labora.com, asmadeus@...ewreck.org, corbet@....net,
ericvh@...nel.org, gregkh@...uxfoundation.org,
kernel@...gutronix.de, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
linux_oss@...debyte.com, lucho@...kov.net, v9fs@...ts.linux.dev
Subject: Re: [PATCH v9 2/3] net/9p/usbg: Add new usb gadget function transport
Thanks for the review!
I just send out v10 with your suggestions:
https://lore.kernel.org/all/20240116-ml-topic-u9p-v10-0-a85fdeac2c52@pengutronix.de/
On Fri, Aug 23, 2024 at 11:45:23AM +0200, Christophe JAILLET wrote:
>Le 23/08/2024 à 09:36, Michael Grzeschik a écrit :
>>Add the new gadget function for 9pfs transport. This function is
>>defining an simple 9pfs transport interface that consists of one in and
>>one out endpoint. The endpoints transmit and receive the 9pfs protocol
>>payload when mounting a 9p filesystem over usb.
>>
>>Tested-by: Andrzej Pietrasiewicz <andrzej.p-ZGY8ohtN/8qB+jHODAdFcQ@...lic.gmane.org>
>>Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@...lic.gmane.org>
>>
>>---
>
>Hi,
>
>a few nitpicks below and 1 or 2 real questions.
>
>>+#include <linux/slab.h>
>>+#include <linux/kernel.h>
>>+#include <linux/device.h>
>>+#include <linux/module.h>
>>+#include <linux/err.h>
>>+#include <linux/usb/composite.h>
>>+#include <linux/usb/func_utils.h>
>>+#include <linux/completion.h>
>
>Keep it in alphabetic order?
>
Did that!
>>+
>>+#include <net/9p/9p.h>
>>+#include <net/9p/client.h>
>>+#include <net/9p/transport.h>
>>+
>
>...
>
>>+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;
>>+ struct p9_req_t *p9_tx_req = req->context;
>>+ unsigned long flags;
>>+
>>+ /* reset zero packages */
>>+ req->zero = 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;
>>+ }
>>+
>>+ dev_dbg(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
>>+ ep->name, req->status, req->actual, req->length);
>>+
>>+ spin_lock_irqsave(&usb9pfs->lock, flags);
>>+ WRITE_ONCE(p9_tx_req->status, REQ_STATUS_SENT);
>>+
>>+ p9_req_put(usb9pfs->client, p9_tx_req);
>>+
>>+ spin_unlock_irqrestore(&usb9pfs->lock, flags);
>>+
>>+ complete(&usb9pfs->send);
>>+
>>+ return;
>
>Is it needed?
Nope, gone!
>>+}
>
>...
>
>>+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;
>>+
>>+ 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;
>>+ }
>
>The { } could be removd.
Done!
>>+
>>+ 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);
>>+
>>+ complete(&usb9pfs->received);
>>+
>>+ return;
>
>Is it needed?
No.
>>+}
>
>...
>
>>+static int alloc_requests(struct usb_composite_dev *cdev,
>>+ struct f_usb9pfs *usb9pfs)
>>+{
>>+ int ret = 0;
>>+
>>+ usb9pfs->in_req = usb_ep_alloc_request(usb9pfs->in_ep, GFP_ATOMIC);
>>+ if (!usb9pfs->in_req) {
>>+ ret = -ENOENT;
>>+ goto fail;
>>+ }
>>+
>>+ usb9pfs->out_req = alloc_ep_req(usb9pfs->out_ep, usb9pfs->buflen);
>>+ if (!usb9pfs->out_req) {
>>+ ret = -ENOENT;
>>+ goto fail_in;
>>+ }
>>+
>>+ usb9pfs->in_req->complete = usb9pfs_tx_complete;
>>+ usb9pfs->out_req->complete = usb9pfs_rx_complete;
>>+
>>+ /* length will be set in complete routine */
>>+ usb9pfs->in_req->context = usb9pfs;
>>+ usb9pfs->out_req->context = usb9pfs;
>>+
>>+ return ret;
>
>return 0; to be more explicit?
>(and would avoid the = 0 when declared)
Good point. Did that.
>>+
>>+fail_in:
>>+ usb_ep_free_request(usb9pfs->in_ep, usb9pfs->in_req);
>>+fail:
>>+ return ret;
>>+}
>>+
>>+static int enable_endpoint(struct usb_composite_dev *cdev,
>>+ struct f_usb9pfs *usb9pfs, struct usb_ep *ep)
>>+{
>>+ int ret;
>>+
>>+ ret = config_ep_by_speed(cdev->gadget, &usb9pfs->function, ep);
>>+ if (ret)
>>+ return ret;
>>+
>>+ ret = usb_ep_enable(ep);
>>+ if (ret < 0)
>>+ return ret;
>>+
>>+ ep->driver_data = usb9pfs;
>>+
>>+ return ret;
>
>return 0; to be more explicit?
Agreed!
>>+}
>
>...
>
>>+static int p9_usbg_create(struct p9_client *client, const char *devname, char *args)
>>+{
>>+ struct f_usb9pfs_dev *dev;
>>+ struct f_usb9pfs_dev *tmp;
>>+ struct f_usb9pfs *usb9pfs;
>>+ struct usb_function *f;
>>+ int ret = -ENOENT;
>>+ int found = 0;
>>+
>>+ if (!devname)
>>+ return -EINVAL;
>>+
>>+ mutex_lock(&usb9pfs_lock);
>
>Using cleanup.h would simplify locking in early exit paths.
Whoo, cleanup.h is a peace of art. I totally forgot about that.
I also addressed the the spinlocks which seemed obvious.
>>+ list_for_each_entry_safe(dev, tmp, &usbg_instance_list, usb9pfs_instance) {
>
>Why the _safe version is used here?
>The list is not modify here directly.
No need. I used the non safe version instead.
>>+ if (!strncmp(devname, dev->tag, strlen(devname))) {
>>+ if (!dev->inuse) {
>>+ dev->inuse = true;
>>+ found = 1;
>>+ break;
>>+ }
>>+ ret = -EBUSY;
>>+ break;
>>+ }
>>+ }
>>+
>>+ if (!found) {
>>+ mutex_unlock(&usb9pfs_lock);
>>+ pr_err("no channels available for device %s\n", devname);
>>+ return ret;
>>+ }
>>+
>>+ usb9pfs = dev->usb9pfs;
>>+ if (!usb9pfs) {
>>+ mutex_unlock(&usb9pfs_lock);
>>+ return -EINVAL;
>>+ }
>>+
>>+ f = &usb9pfs->function;
>>+
>>+ client->trans = (void *)usb9pfs;
>>+ if (!usb9pfs->in_req)
>>+ client->status = Disconnected;
>>+ else
>>+ client->status = Connected;
>>+ usb9pfs->client = client;
>>+
>>+ client->trans_mod->maxsize = usb9pfs->buflen;
>>+
>>+ complete(&usb9pfs->received);
>>+ mutex_unlock(&usb9pfs_lock);
>>+
>>+ return 0;
>>+}
>
>...
>
>>+static ssize_t f_usb9pfs_opts_buflen_show(struct config_item *item, char *page)
>>+{
>>+ struct f_usb9pfs_opts *opts = to_f_usb9pfs_opts(item);
>>+ int ret;
>>+
>>+ mutex_lock(&opts->lock);
>>+ ret = sprintf(page, "%d\n", opts->buflen);
>
>sysfs_emit()?
>
Good point!
>>+ mutex_unlock(&opts->lock);
>>+
>>+ return ret;
>>+}
>
>...
>
>>+static struct f_usb9pfs_dev *_usb9pfs_do_find_dev(const char *tag)
>>+{
>>+ struct f_usb9pfs_dev *usb9pfs_dev;
>>+ struct f_usb9pfs_dev *tmp;
>>+
>>+ if (!tag)
>>+ return NULL;
>>+
>>+ list_for_each_entry_safe(usb9pfs_dev, tmp, &usbg_instance_list, usb9pfs_instance) {
>
>Why the _safe version is used here?
>The list is not modify here directly.
No need for _safe. I switched back to the generic version.
>>+ if (strcmp(usb9pfs_dev->tag, tag) == 0)
>>+ return usb9pfs_dev;
>>+ }
>>+
>>+ return NULL;
>>+}
>
>...
>
>>+static void usb9pfs_free_instance(struct usb_function_instance *fi)
>>+{
>>+ struct f_usb9pfs_opts *usb9pfs_opts =
>>+ container_of(fi, struct f_usb9pfs_opts, func_inst);
>>+ struct f_usb9pfs_dev *dev = usb9pfs_opts->dev;
>>+
>>+ list_del(&dev->usb9pfs_instance);
>
>When it is added in the usbg_instance_list list below, it is protected
>by the usb9pfs_lock mutex. Should it be also protected when it is
>removed?
>
Using the mutex here is valid.
>>+ kfree(usb9pfs_opts);
>>+}
>>+
>>+static struct usb_function_instance *usb9pfs_alloc_instance(void)
>>+{
>>+ struct f_usb9pfs_opts *usb9pfs_opts;
>>+ struct f_usb9pfs_dev *dev;
>>+
>>+ usb9pfs_opts = kzalloc(sizeof(*usb9pfs_opts), GFP_KERNEL);
>>+ if (!usb9pfs_opts)
>>+ return ERR_PTR(-ENOMEM);
>>+
>>+ mutex_init(&usb9pfs_opts->lock);
>>+
>>+ usb9pfs_opts->func_inst.set_inst_name = usb9pfs_set_inst_tag;
>>+ usb9pfs_opts->func_inst.free_func_inst = usb9pfs_free_instance;
>>+
>>+ usb9pfs_opts->buflen = DEFAULT_BUFLEN;
>>+
>>+ mutex_lock(&usb9pfs_lock);
>>+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>
>If kzalloc() was called outside of the mutex, it would be slightly
>simpler, IMHO.
Totally. I do the alloc and ..
>
>>+ if (IS_ERR(dev)) {
>>+ mutex_unlock(&usb9pfs_lock);
>>+ kfree(usb9pfs_opts);
>>+ return ERR_CAST(dev);
>>+ }
>>+ list_add_tail(&dev->usb9pfs_instance, &usbg_instance_list);
>>+ mutex_unlock(&usb9pfs_lock);
>>+
>>+ usb9pfs_opts->dev = dev;
>>+ dev->opts = usb9pfs_opts;
>
>'dev' is added to the usbg_instance_list list within a mutex section,
>so it looks possible that this list is also accessed from somewhere
>else.
>
>Would it make sense to initialize dev->opts also within the mutex
>section, to avoid concurrent access to this new item, when dev->opts
>is still NULL?
.. the allocation all before even adding the prepared instance to the
list. This should make it safe. I actually moved the list alternation
to be the last operation in this function.
>>+
>>+ config_group_init_type_name(&usb9pfs_opts->func_inst.group, "",
>>+ &usb9pfs_func_type);
>>+
>>+ return &usb9pfs_opts->func_inst;
>>+}
>
>...
>
>CJ
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists