[PATCH] usb: gadget: f_ecm: serialize notification state with a spinlock

[PATCH] usb: gadget: f_ecm: serialize notification state with a spinlock

[Yu Zhang]

ecm_do_notify() guards re-entry only with an atomic_read() of
notify_count, then fills ecm->notify_req, does atomic_inc() and
usb_ep_queue().  This check-fill-inc-queue sequence is not atomic and
runs from three contexts that share no lock: ecm_notify_complete(),
ecm_open()/ecm_close() (netdev up/down via u_ether) and ecm_set_alt().

When the completion handler decrements notify_count to 0 and re-enters
ecm_do_notify(), a concurrent ecm_close()/ecm_set_alt() can observe
notify_count == 0 in the window between the atomic_dec() and the next
atomic_inc(), and queue the same ecm->notify_req a second time.  The
request is then added to the endpoint queue twice (list_add corruption),
and notify_state is updated concurrently.

struct f_ecm has carried a FIXME about this missing locking.  The sibling
f_ncm already serializes the identical state machine with a per-instance
spinlock.  Do the same in f_ecm: take ecm->lock across the is_open update
and ecm_notify() in open/close/set_alt and across the state update in
ecm_notify_complete(), dropping it only around usb_ep_queue().

Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/usb/gadget/function/f_ecm.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index e495bac..d699832 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -59,9 +59,8 @@ struct f_ecm {
 	atomic_t			notify_count;
 	bool				is_open;
 
-	/* FIXME is_open needs some irq-ish locking
-	 * ... possibly the same as port.ioport
-	 */
+	/* protects the notification state machine */
+	spinlock_t			lock;
 };
 
 static inline struct f_ecm *func_to_ecm(struct usb_function *f)
@@ -414,7 +413,11 @@ static void ecm_do_notify(struct f_ecm *ecm)
 	event->wIndex = cpu_to_le16(ecm->ctrl_id);
 
 	atomic_inc(&ecm->notify_count);
+
+	/* usb_ep_queue() can complete the request re-entrantly */
+	spin_unlock(&ecm->lock);
 	status = usb_ep_queue(ecm->notify, req, GFP_ATOMIC);
+	spin_lock(&ecm->lock);
 	if (status < 0) {
 		atomic_dec(&ecm->notify_count);
 		DBG(cdev, "notify --> %d\n", status);
@@ -438,6 +441,7 @@ static void ecm_notify_complete(struct usb_ep *ep, struct usb_request *req)
 	struct usb_composite_dev	*cdev = ecm->port.func.config->cdev;
 	struct usb_cdc_notification	*event = req->buf;
 
+	spin_lock(&ecm->lock);
 	switch (req->status) {
 	case 0:
 		/* no fault */
@@ -455,6 +459,7 @@ static void ecm_notify_complete(struct usb_ep *ep, struct usb_request *req)
 		break;
 	}
 	ecm_do_notify(ecm);
+	spin_unlock(&ecm->lock);
 }
 
 static int ecm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
@@ -588,7 +593,9 @@ static int ecm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		 * follow another (if the first is in flight), and instead
 		 * just guarantee that a speed notification is always sent.
 		 */
+		spin_lock(&ecm->lock);
 		ecm_notify(ecm);
+		spin_unlock(&ecm->lock);
 	} else
 		goto fail;
 
@@ -653,8 +660,10 @@ static void ecm_open(struct gether *geth)
 
 	DBG(ecm->port.func.config->cdev, "%s\n", __func__);
 
+	spin_lock(&ecm->lock);
 	ecm->is_open = true;
 	ecm_notify(ecm);
+	spin_unlock(&ecm->lock);
 }
 
 static void ecm_close(struct gether *geth)
@@ -663,8 +672,10 @@ static void ecm_close(struct gether *geth)
 
 	DBG(ecm->port.func.config->cdev, "%s\n", __func__);
 
+	spin_lock(&ecm->lock);
 	ecm->is_open = false;
 	ecm_notify(ecm);
+	spin_unlock(&ecm->lock);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -942,6 +953,8 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi)
 	if (!ecm)
 		return ERR_PTR(-ENOMEM);
 
+	spin_lock_init(&ecm->lock);
+
 	opts = container_of(fi, struct f_ecm_opts, func_inst);
 	mutex_lock(&opts->lock);
 	opts->refcnt++;
-- 
2.43.0