From patchwork Tue Mar 22 13:51:59 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Seth Forshee X-Patchwork-Id: 600792 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 3qTvK25MXHz9s9Z; Wed, 23 Mar 2016 00:52:38 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical-com.20150623.gappssmtp.com header.i=@canonical-com.20150623.gappssmtp.com header.b=I72iTofh; dkim-atps=neutral Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1aiMjc-0003O1-9y; Tue, 22 Mar 2016 13:52:36 +0000 Received: from mail-oi0-f54.google.com ([209.85.218.54]) by huckleberry.canonical.com with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.76) (envelope-from ) id 1aiMjX-0003N8-5o for kernel-team@lists.ubuntu.com; Tue, 22 Mar 2016 13:52:31 +0000 Received: by mail-oi0-f54.google.com with SMTP id r187so175621156oih.3 for ; Tue, 22 Mar 2016 06:52:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical-com.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id:in-reply-to:references; bh=PfiYuCTGl1Bb/e0Y0MjgB8gOO9EzxbIDcYh6rpMw1PY=; b=I72iTofhkLLDQMEXNoXDS0KGGv+FIV2HFQMMBSkOOEdtFBz352Sfe8eDa0ldYyY4dP au5DVfRoUAPDC+ICSCHqOnFyb55oyxIJgZlUnSmpyjBQVeu4klhnlyWVdmHH9gQfc7xa OzBvVqoyEAoAjLoK5kPWSqzvj66+Q+JwlRZ3TC7vfFeiOOtrmCRBJZ9LY9PRfz4PccoM hlQvDcv2tAMZ03kbtS2YiXXm6tFfju/FETNCXIIW8FYwMdGYB3+XPsepJwDGyr4+y4Kj bLJdXsL6T9g2OYQtkRjsFP3sdNGX/MSaReC3NIzgurIJJkjnE/PUghIT7Z2TIpBnMWVu VcCw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=PfiYuCTGl1Bb/e0Y0MjgB8gOO9EzxbIDcYh6rpMw1PY=; b=mfumqD8QYduSwPre8uHY9JWV3jYorOLJd8Rr2efdXzBqSIOg3biWYV1jHrvSPM7S4D /YDmq0jm9AbFhA/kEa+BM2Ju2hA4jCXpMsBnrsP2HAQywv5QBCuEO8/KwaAuxtR/19EM LVdwfbL7a54kIMof5CyinFWF6tp8mAQKJu9SjTwkU+uw2Sbc2YRJHHhUwOqM/8VhtY3f Tz9IKt04teN59gX1iNRRDTV3b3q/SPLjxflsS+4bAV1OD1oplU+1n26GRjf0uCCH9xfc loQ2ZQ43O8bQ+wE2J6wDtFBuyn2hsg7GwMfqPScPq0WSo6g68GpnzZnOcVOc15pDF7N9 vhUQ== X-Gm-Message-State: AD7BkJKm5yqIjNKz9EaPawnHzIe3YpUbkv5acTltuY1PjhvgtTAXFsYrn7Xy644bMh8LHLMc X-Received: by 10.157.34.33 with SMTP id o30mr3299234ota.63.1458654749905; Tue, 22 Mar 2016 06:52:29 -0700 (PDT) Received: from localhost ([2605:a601:aab:f920:f5cc:f15e:6a31:a2c0]) by smtp.gmail.com with ESMTPSA id dg3sm13441796oeb.12.2016.03.22.06.52.29 for (version=TLS1_2 cipher=AES128-SHA bits=128/128); Tue, 22 Mar 2016 06:52:29 -0700 (PDT) From: Seth Forshee To: kernel-team@lists.ubuntu.com Subject: [PATCH 2/2][wily] fuse: Add reference counting for fuse_io_priv Date: Tue, 22 Mar 2016 08:51:59 -0500 Message-Id: <1458654721-63028-3-git-send-email-seth.forshee@canonical.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1458654721-63028-1-git-send-email-seth.forshee@canonical.com> References: <1458654721-63028-1-git-send-email-seth.forshee@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com BugLink: http://bugs.launchpad.net/bugs/1505948 The 'reqs' member of fuse_io_priv serves two purposes. First is to track the number of oustanding async requests to the server and to signal that the io request is completed. The second is to be a reference count on the structure to know when it can be freed. For sync io requests these purposes can be at odds. fuse_direct_IO() wants to block until the request is done, and since the signal is sent when 'reqs' reaches 0 it cannot keep a reference to the object. Yet it needs to use the object after the userspace server has completed processing requests. This leads to some handshaking and special casing that it needlessly complicated and responsible for at least one race condition. It's much cleaner and safer to maintain a separate reference count for the object lifecycle and to let 'reqs' just be a count of outstanding requests to the userspace server. Then we can know for sure when it is safe to free the object without any handshaking or special cases. The catch here is that most of the time these objects are stack allocated and should not be freed. Initializing these objects with a single reference that is never released prevents accidental attempts to free the objects. Fixes: 9d5722b7777e ("fuse: handle synchronous iocbs internally") Cc: stable@vger.kernel.org # v4.1+ Signed-off-by: Seth Forshee Signed-off-by: Miklos Szeredi (cherry picked from commit 744742d692e37ad5c20630e57d526c8f2e2fe3c9 git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git) Signed-off-by: Seth Forshee --- fs/fuse/cuse.c | 4 ++-- fs/fuse/file.c | 28 +++++++++++++++++++++------- fs/fuse/fuse_i.h | 9 +++++++++ 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c index 8e3ee19..c5b6b71 100644 --- a/fs/fuse/cuse.c +++ b/fs/fuse/cuse.c @@ -90,7 +90,7 @@ static struct list_head *cuse_conntbl_head(dev_t devt) static ssize_t cuse_read_iter(struct kiocb *kiocb, struct iov_iter *to) { - struct fuse_io_priv io = { .async = 0, .file = kiocb->ki_filp }; + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(kiocb->ki_filp); loff_t pos = 0; return fuse_direct_io(&io, to, &pos, FUSE_DIO_CUSE); @@ -98,7 +98,7 @@ static ssize_t cuse_read_iter(struct kiocb *kiocb, struct iov_iter *to) static ssize_t cuse_write_iter(struct kiocb *kiocb, struct iov_iter *from) { - struct fuse_io_priv io = { .async = 0, .file = kiocb->ki_filp }; + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(kiocb->ki_filp); loff_t pos = 0; /* * No locking or generic_write_checks(), the server is diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 9e80d01..b4aae5a 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -528,6 +528,11 @@ static void fuse_release_user_pages(struct fuse_req *req, int write) } } +static void fuse_io_release(struct kref *kref) +{ + kfree(container_of(kref, struct fuse_io_priv, refcnt)); +} + static ssize_t fuse_get_res_by_io(struct fuse_io_priv *io) { if (io->err) @@ -585,8 +590,9 @@ static void fuse_aio_complete(struct fuse_io_priv *io, int err, ssize_t pos) } io->iocb->ki_complete(io->iocb, res, 0); - kfree(io); } + + kref_put(&io->refcnt, fuse_io_release); } static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req) @@ -613,6 +619,7 @@ static size_t fuse_async_req_send(struct fuse_conn *fc, struct fuse_req *req, size_t num_bytes, struct fuse_io_priv *io) { spin_lock(&io->lock); + kref_get(&io->refcnt); io->size += num_bytes; io->reqs++; spin_unlock(&io->lock); @@ -691,7 +698,7 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode, static int fuse_do_readpage(struct file *file, struct page *page) { - struct fuse_io_priv io = { .async = 0, .file = file }; + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(file); struct inode *inode = page->mapping->host; struct fuse_conn *fc = get_fuse_conn(inode); struct fuse_req *req; @@ -984,7 +991,7 @@ static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file, size_t res; unsigned offset; unsigned i; - struct fuse_io_priv io = { .async = 0, .file = file }; + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(file); for (i = 0; i < req->num_pages; i++) fuse_wait_on_page_writeback(inode, req->pages[i]->index); @@ -1398,7 +1405,7 @@ static ssize_t __fuse_direct_read(struct fuse_io_priv *io, static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to) { - struct fuse_io_priv io = { .async = 0, .file = iocb->ki_filp }; + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb->ki_filp); return __fuse_direct_read(&io, to, &iocb->ki_pos); } @@ -1406,7 +1413,7 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct file *file = iocb->ki_filp; struct inode *inode = file_inode(file); - struct fuse_io_priv io = { .async = 0, .file = file }; + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(file); ssize_t res; if (is_bad_inode(inode)) @@ -2807,6 +2814,7 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter, loff_t offset) if (!io) return -ENOMEM; spin_lock_init(&io->lock); + kref_init(&io->refcnt); io->reqs = 1; io->bytes = -1; io->size = 0; @@ -2830,8 +2838,14 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter, loff_t offset) iov_iter_rw(iter) == WRITE) io->async = false; - if (io->async && is_sync) + if (io->async && is_sync) { + /* + * Additional reference to keep io around after + * calling fuse_aio_complete() + */ + kref_get(&io->refcnt); io->done = &wait; + } if (iov_iter_rw(iter) == WRITE) { ret = fuse_direct_io(io, iter, &pos, FUSE_DIO_WRITE); @@ -2851,7 +2865,7 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter, loff_t offset) ret = fuse_get_res_by_io(io); } - kfree(io); + kref_put(&io->refcnt, fuse_io_release); if (iov_iter_rw(iter) == WRITE) { if (ret > 0) diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 4051131..604cd42 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -22,6 +22,7 @@ #include #include #include +#include /** Max number of pages that can be used in a single read request */ #define FUSE_MAX_PAGES_PER_REQ 32 @@ -243,6 +244,7 @@ struct fuse_args { /** The request IO state (for asynchronous processing) */ struct fuse_io_priv { + struct kref refcnt; int async; spinlock_t lock; unsigned reqs; @@ -256,6 +258,13 @@ struct fuse_io_priv { struct completion *done; }; +#define FUSE_IO_PRIV_SYNC(f) \ +{ \ + .refcnt = { ATOMIC_INIT(1) }, \ + .async = 0, \ + .file = f, \ +} + /** * Request flags *