From patchwork Thu Sep 28 20:19:04 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Avichal Rakesh X-Patchwork-Id: 146342 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp3655713vqu; Thu, 28 Sep 2023 16:06:48 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG9l96xrMVx/qhtJO96y557k7whWuPaG1HOBcUa5pb/rYm+4RZHb0FPp3H6Udo/iOLR5+Yq X-Received: by 2002:a05:6e02:2168:b0:351:4e9f:5606 with SMTP id s8-20020a056e02216800b003514e9f5606mr3348159ilv.10.1695942408150; Thu, 28 Sep 2023 16:06:48 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695942408; cv=none; d=google.com; s=arc-20160816; b=RP4FHnhiutALPFYBh3GWQsZpJjUe7X8jJ4IKWoAqr8M9wzZe0S3obAQKn1eKoiBqyA fMYgIMrUFuxm4WkJQDxeS3J/i5l07l4LXLK64Sn5G4nFId/Mo0IknncCtahrHN3PeqfU DlQ9TbxafcYHlVHCtRFk/6iPOeSiRCIq9q7U4RINN1upmc7+QTZgRdryss03R+rF/xje Psydm2X+AGHpriaNl7fHD21RclKcbMLqAluMygRW5UeVODxVpa+HcnBtUyacqJvI8T60 fyshlpYLI0sGSG/p/Iq+hQqNzQ0zs9dmJjbUtCVXptwyXt2axVqSed7GXKabvDLeKPyQ 9qbA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:dkim-signature; bh=pllQ3EK+t+tR7Fp3wficz5gYikkfu1qqo+IJ9sVnus0=; fh=rOrSvpCIfr8ex8KKB5qAoK2H8XmRXmZqpC7o21WtflE=; b=NT45rtw41kg2yzkY6MTxSBhdHja802Lyu9VGhGpOP/lZfUDTmkZrn2UmltZAiMhwD3 RBzupXZigYu2WB+XVU2f75eWDa00blyP82rsRvMDfoo34PDRYrYTNiM2IYSo+PS49LXY epS5tfFzkAC7FJS/MirVa03GvGpubId48Zcmxmi/IQ9OnsvRyHD1ctMCq+tCBT9Dj61N w1qGIheU53NRrzxy59K6eq6N7KRxg7Pm9oj5cPj8IHMbHN1VfRWXJxvZavGJEXQLr6Em +Omy5v80DYAgfRnxi82sdOis7mOQiXzkCNblLFk4nwcdP9cIFYb3oDgHqDIdwjGcvQs0 vWTQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=Cygn22rM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id h9-20020a636c09000000b0058511efe7d7si7078852pgc.97.2023.09.28.16.06.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Sep 2023 16:06:48 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=Cygn22rM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 26F2482D2928; Thu, 28 Sep 2023 13:19:45 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232081AbjI1UTP (ORCPT + 21 others); Thu, 28 Sep 2023 16:19:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46694 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231446AbjI1UTN (ORCPT ); Thu, 28 Sep 2023 16:19:13 -0400 Received: from mail-yb1-xb49.google.com (mail-yb1-xb49.google.com [IPv6:2607:f8b0:4864:20::b49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 390BD194 for ; Thu, 28 Sep 2023 13:19:11 -0700 (PDT) Received: by mail-yb1-xb49.google.com with SMTP id 3f1490d57ef6-d81646fcf3eso20727346276.0 for ; Thu, 28 Sep 2023 13:19:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1695932350; x=1696537150; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=pllQ3EK+t+tR7Fp3wficz5gYikkfu1qqo+IJ9sVnus0=; b=Cygn22rMUnowgL6axg7X+/22dSxYieotSuUfe+Y8Wl+TJtQjItIgyDwjg8gIcn7ebr zNSBA9lG6y5nAeDVKGN2yqRCLIi3X8kVkxvHLSyK5ul98FvBwgYqIeKE5a9+sf0OVn3y YwP2WM3xUwZ3l56Ol8hId8PKxFXNEnogFwdnYHzsUReDGLbdEWrbi+EaYhv/Xi4vszjP dqdd42Unu+NRXwQ/X1uG56d+AtUX7rQKge4rgFodfN+Rddthhv7dXFivHocSZYaJm2Ik ETiJZMLRGMRlAP65gIu2adXfbjKAF9TN5PpzwsFTHmdO+PS09QMonE0IQrGPlEWiQJ7g pfuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695932350; x=1696537150; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pllQ3EK+t+tR7Fp3wficz5gYikkfu1qqo+IJ9sVnus0=; b=k4/dsnbJtWx0W/6oH4bWASmnXnZVeaGvcatN/1Snz7G3FsCOd8fr0XOtahyotlWGNS LtOuJOLcKxVukNMpTtei0rl9BZXnksqRZvVfkznEP0Q7Hhwac4yqCmVd7cSEnJ3cdWwF cUpCpBjBv2heYpOmS4pZGh75oFt2+STKnfXy2yDwEtLnIvpEswhf1LuENs47cX27OZJE mGMXy2s0OwSxJBva9xnC4WP24zJVsCEDI0Ab30uA+fq1RdS8OACX024o6uryd0feCkiR Myk69wDX5HGmuAA1HqqnFoFEB3sWP30p+x5vnSt+4HYgHmpLyxFKiGhWvUpkddzkH2Ef ynzQ== X-Gm-Message-State: AOJu0Yx60qZAtZ80+sVaRKji8WoGTda7CvmvXXRUQobZnqzzcU/0TiNE t1ojr43rk053h0Z+j5ysmiMKr2exefaz X-Received: from hi-h2o-specialist.c.googlers.com ([fda3:e722:ac3:cc00:24:72f4:c0a8:3cef]) (user=arakesh job=sendgmr) by 2002:a05:6902:181c:b0:d82:9342:8627 with SMTP id cf28-20020a056902181c00b00d8293428627mr34574ybb.6.1695932350374; Thu, 28 Sep 2023 13:19:10 -0700 (PDT) Date: Thu, 28 Sep 2023 13:19:04 -0700 In-Reply-To: <20230920200335.63709-1-arakesh@google.com> Mime-Version: 1.0 References: <20230920200335.63709-1-arakesh@google.com> X-Mailer: git-send-email 2.42.0.582.g8ccd20d70d-goog Message-ID: <20230928201904.454229-1-arakesh@google.com> Subject: [PATCH v3 2/2] usb: gadget: uvc: prevent de-allocating inflight usb_requests From: Avichal Rakesh To: arakesh@google.com Cc: dan.scally@ideasonboard.com, etalvala@google.com, gregkh@linuxfoundation.org, jchowdhary@google.com, laurent.pinchart@ideasonboard.com, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, m.grzeschik@pengutronix.de, mgr@pengutronix.de X-Spam-Status: No, score=-5.9 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SORTED_RECIPS,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 28 Sep 2023 13:19:45 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1776810623544480477 X-GMAIL-MSGID: 1778324506152858790 Currently, when stopping the stream, uvcg_video_enable immediately deallocates the usb_requests after calling usb_ep_dequeue. However, usb_ep_dequeue is asynchronous and it is possible that it a request that is still in use by the controller. This can lead to some hard to reproduce crashes with double frees and general memory inconsistencies. This patch sets up some stronger guarantees around when a request should be deallocated. To that extent, this patch does the following: 1. When stream is stopped only the currently owned uvc_requests are freed and all in-flight uvc_requests are marked 'abandoned' 2. uvc_video_complete callback is made responsible for freeing up the abandoned requests. No uvc specific logic is triggered when handling abandoned requests. This ensures that the ownership of uvc_request (and its corresponding usb_request) is never ambiguous, and uvc_requests are always freed regardless of when the requests are returned to the gadget driver. Other changes in the patch are required to decouple the allocated uvc_requests from uvc_video that allocated them. Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com Suggested-by: Michael Grzeschik Signed-off-by: Avichal Rakesh --- v1 -> v2: Switched from a timed wait to free on complete model based on discussions with Michael. v3 -> v3: Initialized ureqs at init to prevent accessing invalid memory when no stream is created. drivers/usb/gadget/function/uvc.h | 4 +- drivers/usb/gadget/function/uvc_video.c | 183 +++++++++++++++++------- 2 files changed, 134 insertions(+), 53 deletions(-) -- 2.42.0.582.g8ccd20d70d-goog diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 989bc6b4e93d..e69cfb7cced1 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -81,6 +81,8 @@ struct uvc_request { struct sg_table sgt; u8 header[UVCG_REQUEST_HEADER_LEN]; struct uvc_buffer *last_buf; + struct list_head list; + bool is_abandoned; }; struct uvc_video { @@ -102,7 +104,7 @@ struct uvc_video { /* Requests */ unsigned int req_size; - struct uvc_request *ureq; + struct list_head ureqs; /* all uvc_requests allocated by uvc_video */ struct list_head req_free; spinlock_t req_lock; diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index 70ff88854539..aa5ef63e5540 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -227,6 +227,23 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, * Request handling */ +static void uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) +{ + sg_free_table(&ureq->sgt); + if (ureq->req && ep) { + usb_ep_free_request(ep, ureq->req); + ureq->req = NULL; + } + + kfree(ureq->req_buffer); + ureq->req_buffer = NULL; + + if (!list_empty(&ureq->list)) + list_del(&ureq->list); + + kfree(ureq); +} + static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) { int ret; @@ -254,7 +271,21 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) struct uvc_video *video = ureq->video; struct uvc_video_queue *queue = &video->queue; struct uvc_device *uvc = video->uvc; + struct uvc_buffer *last_buf; unsigned long flags; + bool is_abandoned; + + spin_lock_irqsave(&video->req_lock, flags); + is_abandoned = ureq->is_abandoned; + last_buf = ureq->last_buf; + ureq->last_buf = NULL; + spin_unlock_irqrestore(&video->req_lock, flags); + + if (is_abandoned) { + uvcg_dbg(&video->uvc->func, "Freeing abandoned request\n"); + uvc_video_free_request(ureq, ep); + return; + } switch (req->status) { case 0: @@ -277,15 +308,27 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) uvcg_queue_cancel(queue, 0); } - if (ureq->last_buf) { - uvcg_complete_buffer(&video->queue, ureq->last_buf); - ureq->last_buf = NULL; + if (last_buf) { + spin_lock_irqsave(&video->queue.irqlock, flags); + uvcg_complete_buffer(&video->queue, last_buf); + spin_unlock_irqrestore(&video->queue.irqlock, flags); } + /* + * request might have been abandoned while being processed. + * do a last minute check before queueing the request back. + */ spin_lock_irqsave(&video->req_lock, flags); - list_add_tail(&req->list, &video->req_free); + is_abandoned = ureq->is_abandoned; + if (!is_abandoned) + list_add_tail(&req->list, &video->req_free); spin_unlock_irqrestore(&video->req_lock, flags); + if (is_abandoned) { + uvc_video_free_request(ureq, ep); + return; + } + if (uvc->state == UVC_STATE_STREAMING) queue_work(video->async_wq, &video->pump); } @@ -293,25 +336,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) static int uvc_video_free_requests(struct uvc_video *video) { - unsigned int i; - - if (video->ureq) { - for (i = 0; i < video->uvc_num_requests; ++i) { - sg_free_table(&video->ureq[i].sgt); + struct uvc_request *ureq, *temp; - if (video->ureq[i].req) { - usb_ep_free_request(video->ep, video->ureq[i].req); - video->ureq[i].req = NULL; - } - - if (video->ureq[i].req_buffer) { - kfree(video->ureq[i].req_buffer); - video->ureq[i].req_buffer = NULL; - } - } - - kfree(video->ureq); - video->ureq = NULL; + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { + uvc_video_free_request(ureq, video->ep); } INIT_LIST_HEAD(&video->req_free); @@ -322,6 +350,7 @@ uvc_video_free_requests(struct uvc_video *video) static int uvc_video_alloc_requests(struct uvc_video *video) { + struct uvc_request *ureq; unsigned int req_size; unsigned int i; int ret = -ENOMEM; @@ -332,35 +361,36 @@ uvc_video_alloc_requests(struct uvc_video *video) * max_t(unsigned int, video->ep->maxburst, 1) * (video->ep->mult); - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); - if (video->ureq == NULL) - return -ENOMEM; - - for (i = 0; i < video->uvc_num_requests; ++i) { - video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL); - if (video->ureq[i].req_buffer == NULL) + INIT_LIST_HEAD(&video->ureqs); + for (i = 0; i < video->uvc_num_requests; i++) { + ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); + if (ureq == NULL) goto error; + INIT_LIST_HEAD(&ureq->list); + list_add_tail(&ureq->list, &video->ureqs); + } - video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL); - if (video->ureq[i].req == NULL) + list_for_each_entry(ureq, &video->ureqs, list) { + ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); + if (ureq->req_buffer == NULL) goto error; - - video->ureq[i].req->buf = video->ureq[i].req_buffer; - video->ureq[i].req->length = 0; - video->ureq[i].req->complete = uvc_video_complete; - video->ureq[i].req->context = &video->ureq[i]; - video->ureq[i].video = video; - video->ureq[i].last_buf = NULL; - - list_add_tail(&video->ureq[i].req->list, &video->req_free); + ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL); + if (ureq->req == NULL) + goto error; + ureq->req->buf = ureq->req_buffer; + ureq->req->length = 0; + ureq->req->complete = uvc_video_complete; + ureq->req->context = ureq; + ureq->video = video; + ureq->last_buf = NULL; + list_add_tail(&ureq->req->list, &video->req_free); /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ - sg_alloc_table(&video->ureq[i].sgt, + sg_alloc_table(&ureq->sgt, DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, PAGE_SIZE) + 2, GFP_KERNEL); } video->req_size = req_size; - return 0; error: @@ -484,12 +514,68 @@ static void uvcg_video_pump(struct work_struct *work) return; } +/* + * Disable video stream. This ensures that any inflight usb requests are marked + * for clean up and video buffers are dropped up before returning. + */ +static void uvcg_video_disable(struct uvc_video *video) +{ + struct uvc_buffer *buf, *tmp_buf; + struct uvc_request *ureq, *temp; + struct list_head buf_list; /* track in-flight video buffers */ + struct usb_request *req; + unsigned long flags; + + cancel_work_sync(&video->pump); + uvcg_queue_cancel(&video->queue, 0); + + INIT_LIST_HEAD(&buf_list); + spin_lock_irqsave(&video->req_lock, flags); + /* abandon all usb requests */ + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { + ureq->is_abandoned = true; + if (ureq->last_buf) { + list_add(&ureq->last_buf->queue, &buf_list); + ureq->last_buf = NULL; + } + list_del_init(&ureq->list); + if (ureq->req) + usb_ep_dequeue(video->ep, ureq->req); + } + /* + * re-add uvc_requests currently owned by the gadget to + * video->ureqs to be deallocated + */ + list_for_each_entry(req, &video->req_free, list) { + ureq = req->context; + list_add_tail(&ureq->list, &video->ureqs); + } + spin_unlock_irqrestore(&video->req_lock, flags); + + /* + * drop abandoned uvc_buffers, as the completion handler + * no longer will + */ + if (!list_empty(&buf_list)) { + spin_lock_irqsave(&video->queue.irqlock, flags); + list_for_each_entry_safe(buf, tmp_buf, + &buf_list, queue) { + video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE; + uvcg_complete_buffer(&video->queue, buf); + list_del(&buf->queue); + } + spin_unlock_irqrestore(&video->queue.irqlock, flags); + } + + uvc_video_free_requests(video); + uvcg_queue_enable(&video->queue, 0); +} + /* * Enable or disable the video stream. */ int uvcg_video_enable(struct uvc_video *video, int enable) { - unsigned int i; int ret; if (video->ep == NULL) { @@ -499,15 +585,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) } if (!enable) { - cancel_work_sync(&video->pump); - uvcg_queue_cancel(&video->queue, 0); - - for (i = 0; i < video->uvc_num_requests; ++i) - if (video->ureq && video->ureq[i].req) - usb_ep_dequeue(video->ep, video->ureq[i].req); - - uvc_video_free_requests(video); - uvcg_queue_enable(&video->queue, 0); + uvcg_video_disable(video); return 0; } @@ -536,6 +614,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) */ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) { + INIT_LIST_HEAD(&video->ureqs); INIT_LIST_HEAD(&video->req_free); spin_lock_init(&video->req_lock); INIT_WORK(&video->pump, uvcg_video_pump);