Message ID | tencent_DC4C9767C786D2D2FDC64F099FAEFDEEC106@qq.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-89730-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:fa17:b0:10a:f01:a869 with SMTP id ju23csp890404dyc; Sun, 3 Mar 2024 04:21:46 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCUNuJScyvAJbLoy3gWGzMFIou/EvxIyL5hJuSs364+xf73Wz+HYApXmF01pMJ0eHrrdix0m+i3VINtukxdD77GKC1WAvA== X-Google-Smtp-Source: AGHT+IFdOoEvJe9oD4FwvZlIzI0fApCMNlIcdUvzlZsNG3DFzfrj13tc4Ts1PFNasiqDhvjAWLQ7 X-Received: by 2002:a05:6a21:196:b0:1a1:4d85:5838 with SMTP id le22-20020a056a21019600b001a14d855838mr291626pzb.22.1709468506622; Sun, 03 Mar 2024 04:21:46 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709468506; cv=pass; d=google.com; s=arc-20160816; b=f3bg21GVbaGaRRuTx4wnn9cy0mjRCb3baSWsRDResExEMFwh8i+jZYs5G+6d9sZis4 gibmSGmkt62qJKv2Whahp/bIn4A3v6SpCTQqSjkPTXSr/xSHIZKuai0fvczysQfLkwVQ 7D6c9/RGhUrCyq6AeE13iEpySYuyLwtQHSYU8D40ez7oqAVl9tEwsobupRZgHi8z8fcW +6pPNVoDhiG1nZyH6PoxYEWqffyLAOZDr8xt6mqBvuop5ZSJKnPOieItcnYw7azqxh5v tnraeouCjf4T3DZG2uGQG4+torNyC2PCX/PqFc5EuavgS72uOF92FZOGJrQrxUFFwbLk DRAQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:date :subject:cc:to:from:message-id:dkim-signature; bh=CgOhE/dgm6I4NvX+ZRJHniVtwsR+cz4XQAYa5Q04GXQ=; fh=ZLq8tcLOM/bayW1JnRC4lOXPDd1ZdYtkdm4/qv6cNsU=; b=rWgnWg9MeDenKCW/vxiSdO3SNjY2wwvkB68675m5oTcII/M++B55knC18sQzJ6XPy/ or+4aZQIWq6kngXMabTZp7X1KMv3lIFkMqDSf8rQv/ZeePeFIXsY9e9+gltLHQKBKSwg n0xsPePB58jPhFwM/3cPZR6SRNxzGp1M+VUT952gZPkkBeps8HI1xAL7g7cxZ36eAODR axUtVsRSxSDHwn2jUPQ5K+ZwdOib3NqdzcQ9ujugGMR+WEfBCcQfxpzP7Y52OY5jhp15 cYT/nCpGahuonqFL0QJY67DKx8Z/KAevZj7IE4bt+JzHHMQV6RJ41ikf6lxam5dAvXjd 4NLA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@qq.com header.s=s201512 header.b=VIo9HtUj; arc=pass (i=1 spf=pass spfdomain=qq.com dkim=pass dkdomain=qq.com dmarc=pass fromdomain=qq.com); spf=pass (google.com: domain of linux-kernel+bounces-89730-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-89730-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=qq.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id v2-20020a17090abb8200b0029a23493f23si8759227pjr.73.2024.03.03.04.21.46 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Mar 2024 04:21:46 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-89730-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@qq.com header.s=s201512 header.b=VIo9HtUj; arc=pass (i=1 spf=pass spfdomain=qq.com dkim=pass dkdomain=qq.com dmarc=pass fromdomain=qq.com); spf=pass (google.com: domain of linux-kernel+bounces-89730-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-89730-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=qq.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 712AC2839A9 for <ouuuleilei@gmail.com>; Sun, 3 Mar 2024 12:21:46 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id DE01CC2E9; Sun, 3 Mar 2024 12:21:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=qq.com header.i=@qq.com header.b="VIo9HtUj" Received: from out203-205-221-153.mail.qq.com (out203-205-221-153.mail.qq.com [203.205.221.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6F872111A4; Sun, 3 Mar 2024 12:21:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=203.205.221.153 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709468489; cv=none; b=ClyFoxwSxWlV0C7vnyFSyD0fy/cLodeevB7G4n7PPk3X2Ln/TYxHVkOcVnpQDzsLkv9RBO7ZdXHi0bRYojLSQshsZmwfFaiA8MtOu2OGFSVB9nRIrVCuvrBNHq1QNzaoDLqaJ3Jmdm38h0EawkVAO4WI43cvwB/l075vTSFse5s= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709468489; c=relaxed/simple; bh=zwHRSE399zvO14wNCoVjD2Iw+mMHJViGTEUJcbdJnik=; h=Message-ID:From:To:Cc:Subject:Date:In-Reply-To:References: MIME-Version; b=ovw3PwFp515fRhZ1vVDK2j8O+3mBtkKNbmCRFIdApkJwBqLUbP5cPNQeJgiDwwzYKIXuJzG1Qu5r53DOwRMnopOY1VWHQ3V5mtlGQXi+0MKuYTP0V+TCnYUKogFn8496RdB74GfFxu74BG4Mo1/780RK9be7ADbXqoWGPpZI1yg= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=qq.com; spf=pass smtp.mailfrom=qq.com; dkim=pass (1024-bit key) header.d=qq.com header.i=@qq.com header.b=VIo9HtUj; arc=none smtp.client-ip=203.205.221.153 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=qq.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=qq.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qq.com; s=s201512; t=1709468483; bh=CgOhE/dgm6I4NvX+ZRJHniVtwsR+cz4XQAYa5Q04GXQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=VIo9HtUjTt2SL3G60KSGA0CcI8eO0dXszUBceelOlP4/dLNXxnI+IyUdnSWcn7dxJ hOVKto/t7GX3O6q7+icP8Z8SHPz6Tdw3rtDD/ZsQwlt9ybtAUf7aIA4k4nf42WPr4C Ip+MYMHsusGbb61vyWj1O9tNhv/JZhNTu4qCT34s= Received: from pek-lxu-l1.wrs.com ([111.198.228.140]) by newxmesmtplogicsvrszc5-2.qq.com (NewEsmtp) with SMTP id 54012E0D; Sun, 03 Mar 2024 20:21:00 +0800 X-QQ-mid: xmsmtpt1709468460t50or4z1m Message-ID: <tencent_DC4C9767C786D2D2FDC64F099FAEFDEEC106@qq.com> X-QQ-XMAILINFO: N26DAMVpW7UEwL1zQa/2t7K3v/lWcNbP/+of6Pr07iOWZgvklSOgghBMArXiS4 ZNScbdHzEdlndADc81nmIU73JFwuquPw9n9VOOj0T62/OwbVOGkFu1gGeafReejV87r8cM/+wkcU c6+8dp5I9vFOgDKDaSjmCR7wN6UHBhF4FqaIBYhaVDpt0gp7MhBNq0EH67CVMwz0F7KyN1L/NLeb 9d7Kh1cMEhslvO09j8mcYV05ov7lQDj5bcCxiql0XpfOh58Ysq9k9H78Idxs19Bb8nFgGxjfT8m0 zzbk0fIKtLQIxsnar3NgOkm9L1zqAO/g4TTUUbBgZUEVVLJNcswqnJcVNYZ79V2bqL3fO5703zRY o9HZFkOeSwDEK/+66pKf8Ty2Y1TlExBsFFH/cBkmJrh/14FM2mqNwC9ghpUZPkLvFnXpmSafEyu+ wFuygUAQ+7pqtYPv6PcZHlM8bPozmxQeireOb80IPPgTzQtncomRD7H4HqQWNQZL+e4AEJ+5I5B1 E3r1WR8v1EIvIwmA5HcDmzmr5X/Gv5/bVuKaFrOhPhkmsAJLNS2iZod35H3Z5ltYD+LIrDUIAFi1 IUI2fyyPj+j4VIihbj6YevNWLINXTuQl47nXwXumzKrACXZ1KE5A9LRALucTZeTMuPG6QsRPhaKT sqJixTDwT5I3JC7pQBTK41+D/ovv+ove2ab758yzLxZedcl2IivD23KX//933zGgP1VkgfdP6Qp7 OcLOf8aXRvGaM0ifmy/SfpNKdialIAi6/ly4cn2u7tCcru849zTa41FAomTMp1PlCPchxVkWF+t9 8gB9dMmqdXTsEQfnpIYohn8Jw0A0wa7z/LSsHeKh8csTzoxcifyMXURxvbGLJGI8PwC9ynjOHqPb yQ6u+WJii/nHjx6JLjwclHCrLdIHXKcWXshJ7Qvjs4tNt6D0Rictts5BAku2VonGESkfMC2kUhb7 8fUVjqntw= X-QQ-XMRINFO: OWPUhxQsoeAVDbp3OJHYyFg= From: Edward Adam Davis <eadavis@qq.com> To: syzbot+b91eb2ed18f599dd3c31@syzkaller.appspotmail.com Cc: bcrl@kvack.org, brauner@kernel.org, bvanassche@acm.org, jack@suse.cz, linux-aio@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, viro@zeniv.linux.org.uk Subject: [PATCH] fs/aio: fix uaf in sys_io_cancel Date: Sun, 3 Mar 2024 20:21:00 +0800 X-OQ-MSGID: <20240303122059.579229-2-eadavis@qq.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <0000000000006945730612bc9173@google.com> References: <0000000000006945730612bc9173@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792507648309284617 X-GMAIL-MSGID: 1792507648309284617 |
Series |
fs/aio: fix uaf in sys_io_cancel
|
|
Commit Message
Edward Adam Davis
March 3, 2024, 12:21 p.m. UTC
The aio poll work aio_poll_complete_work() need to be synchronized with syscall
io_cancel(). Otherwise, when poll work executes first, syscall may access the
released aio_kiocb object.
Fixes: 54cbc058d86b ("fs/aio: Make io_cancel() generate completions again")
Reported-and-tested-by: syzbot+b91eb2ed18f599dd3c31@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
fs/aio.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Comments
On 3/3/24 04:21, Edward Adam Davis wrote: > The aio poll work aio_poll_complete_work() need to be synchronized with syscall > io_cancel(). Otherwise, when poll work executes first, syscall may access the > released aio_kiocb object. > > Fixes: 54cbc058d86b ("fs/aio: Make io_cancel() generate completions again") > Reported-and-tested-by: syzbot+b91eb2ed18f599dd3c31@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/aio.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/aio.c b/fs/aio.c > index 28223f511931..0fed22ed9eb8 100644 > --- a/fs/aio.c > +++ b/fs/aio.c > @@ -1762,9 +1762,8 @@ static void aio_poll_complete_work(struct work_struct *work) > } /* else, POLLFREE has freed the waitqueue, so we must complete */ > list_del_init(&iocb->ki_list); > iocb->ki_res.res = mangle_poll(mask); > - spin_unlock_irq(&ctx->ctx_lock); > - > iocb_put(iocb); > + spin_unlock_irq(&ctx->ctx_lock); > } > > /* assumes we are called with irqs disabled */ > @@ -2198,7 +2197,6 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, > break; > } > } > - spin_unlock_irq(&ctx->ctx_lock); > > /* > * The result argument is no longer used - the io_event is always > @@ -2206,6 +2204,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, > */ > if (ret == 0 && kiocb->rw.ki_flags & IOCB_AIO_RW) > aio_complete_rw(&kiocb->rw, -EINTR); > + spin_unlock_irq(&ctx->ctx_lock); > > percpu_ref_put(&ctx->users); I'm not enthusiast about the above patch because it increases the amount of code executed with the ctx_lock held. Wouldn't something like the untested patch below be a better solution? Thanks, Bart. diff --git a/fs/aio.c b/fs/aio.c index 28223f511931..c6fb10321e48 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -2177,6 +2177,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, struct kioctx *ctx; struct aio_kiocb *kiocb; int ret = -EINVAL; + bool is_cancelled_rw = false; u32 key; u64 obj = (u64)(unsigned long)iocb; @@ -2193,6 +2194,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, /* TODO: use a hash or array, this sucks. */ list_for_each_entry(kiocb, &ctx->active_reqs, ki_list) { if (kiocb->ki_res.obj == obj) { + is_cancelled_rw = kiocb->rw.ki_flags & IOCB_AIO_RW; ret = kiocb->ki_cancel(&kiocb->rw); list_del_init(&kiocb->ki_list); break; @@ -2204,7 +2206,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, * The result argument is no longer used - the io_event is always * delivered via the ring buffer. */ - if (ret == 0 && kiocb->rw.ki_flags & IOCB_AIO_RW) + if (ret == 0 && is_cancelled_rw) aio_complete_rw(&kiocb->rw, -EINTR); percpu_ref_put(&ctx->users);
On Mon, Mar 04, 2024 at 08:15:15AM -0800, Bart Van Assche wrote: > On 3/3/24 04:21, Edward Adam Davis wrote: > >The aio poll work aio_poll_complete_work() need to be synchronized with > >syscall > >io_cancel(). Otherwise, when poll work executes first, syscall may access > >the > >released aio_kiocb object. > > > >Fixes: 54cbc058d86b ("fs/aio: Make io_cancel() generate completions again") > >Reported-and-tested-by: > >syzbot+b91eb2ed18f599dd3c31@syzkaller.appspotmail.com > >Signed-off-by: Edward Adam Davis <eadavis@qq.com> > >--- > > fs/aio.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > >diff --git a/fs/aio.c b/fs/aio.c > >index 28223f511931..0fed22ed9eb8 100644 > >--- a/fs/aio.c > >+++ b/fs/aio.c > >@@ -1762,9 +1762,8 @@ static void aio_poll_complete_work(struct > >work_struct *work) > > } /* else, POLLFREE has freed the waitqueue, so we must complete */ > > list_del_init(&iocb->ki_list); > > iocb->ki_res.res = mangle_poll(mask); > >- spin_unlock_irq(&ctx->ctx_lock); > >- > > iocb_put(iocb); > >+ spin_unlock_irq(&ctx->ctx_lock); > > } > > > > /* assumes we are called with irqs disabled */ > >@@ -2198,7 +2197,6 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, > >struct iocb __user *, iocb, > > break; > > } > > } > >- spin_unlock_irq(&ctx->ctx_lock); > > > > /* > > * The result argument is no longer used - the io_event is always > >@@ -2206,6 +2204,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, > >struct iocb __user *, iocb, > > */ > > if (ret == 0 && kiocb->rw.ki_flags & IOCB_AIO_RW) > > aio_complete_rw(&kiocb->rw, -EINTR); > >+ spin_unlock_irq(&ctx->ctx_lock); > > > > percpu_ref_put(&ctx->users); This is just so wrong there aren't even words to describe it. I recommending reverting all of Bart's patches since they were not reviewed by anyone with a sufficient level of familiarity with fs/aio.c to get it right. -ben > I'm not enthusiast about the above patch because it increases the amount > of code executed with the ctx_lock held. Wouldn't something like the > untested patch below be a better solution? > > Thanks, > > Bart. > > > diff --git a/fs/aio.c b/fs/aio.c > index 28223f511931..c6fb10321e48 100644 > --- a/fs/aio.c > +++ b/fs/aio.c > @@ -2177,6 +2177,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, > struct iocb __user *, iocb, > struct kioctx *ctx; > struct aio_kiocb *kiocb; > int ret = -EINVAL; > + bool is_cancelled_rw = false; > u32 key; > u64 obj = (u64)(unsigned long)iocb; > > @@ -2193,6 +2194,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, > struct iocb __user *, iocb, > /* TODO: use a hash or array, this sucks. */ > list_for_each_entry(kiocb, &ctx->active_reqs, ki_list) { > if (kiocb->ki_res.obj == obj) { > + is_cancelled_rw = kiocb->rw.ki_flags & IOCB_AIO_RW; > ret = kiocb->ki_cancel(&kiocb->rw); > list_del_init(&kiocb->ki_list); > break; > @@ -2204,7 +2206,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, > struct iocb __user *, iocb, > * The result argument is no longer used - the io_event is always > * delivered via the ring buffer. > */ > - if (ret == 0 && kiocb->rw.ki_flags & IOCB_AIO_RW) > + if (ret == 0 && is_cancelled_rw) > aio_complete_rw(&kiocb->rw, -EINTR); > > percpu_ref_put(&ctx->users); > >
On 3/4/24 09:03, Benjamin LaHaise wrote: > This is just so wrong there aren't even words to describe it. I > recommending reverting all of Bart's patches since they were not reviewed > by anyone with a sufficient level of familiarity with fs/aio.c to get it > right. Where were you while my patches were posted for review on the fsdevel mailing list and before these were sent to Linus? A revert request should include a detailed explanation of why the revert should happen. Just claiming that something is wrong is not sufficient to motivate a revert. Bart.
On Mon, Mar 04, 2024 at 09:15:47AM -0800, Bart Van Assche wrote: > On 3/4/24 09:03, Benjamin LaHaise wrote: > >This is just so wrong there aren't even words to describe it. I > >recommending reverting all of Bart's patches since they were not reviewed > >by anyone with a sufficient level of familiarity with fs/aio.c to get it > >right. > > Where were you while my patches were posted for review on the fsdevel > mailing list and before these were sent to Linus? That doesn't negate the need for someone with experience in a given subsystem to sign off on the patches. There are at least 2 other people I would trust to properly review these patches, and none of their signoffs are present either. > A revert request should include a detailed explanation of why the revert > should happen. Just claiming that something is wrong is not sufficient > to motivate a revert. A revert is justified when a series of patches is buggy and had insufficient review prior to merging. Mainline is not the place to be testing half baked changes. Getting cancellation semantics and locking right is *VERY HARD*, and the results of getting it wrong are very subtle and user exploitable. Using the "a kernel warning hit" approach for work on cancellation is very much a sign that the patches were half baked. What testing did you do on your patch series? The commit messages show nothing of interest regarding testing. Why are you touching the kiocb after ownership has already been passed on to another entity? This is as bad as touching memory that has been freed. You clearly do not understand how that code works. -ben
On 3/4/24 09:31, Benjamin LaHaise wrote: > A revert is justified when a series of patches is buggy and had > insufficient review prior to merging. That's not how Linux kernel development works. If a bug can get fixed easily, a fix is preferred instead of reverting + reapplying a patch. > Using the "a kernel warning hit" approach for work on cancellation is > very much a sign that the patches were half baked. Is there perhaps a misunderstanding? My patches fix a kernel warning and did not introduce any new WARN*() statements. > Why are you touching the kiocb after ownership has already been > passed on to another entity? Touching the kiocb after ownership has been passed is the result of an oversight. Whether or not kiocb->ki_cancel() transfers ownership depends on the I/O type. The use-after-free was not introduced on purpose. Bart.
On Mon, Mar 04, 2024 at 09:40:35AM -0800, Bart Van Assche wrote: > On 3/4/24 09:31, Benjamin LaHaise wrote: > >A revert is justified when a series of patches is buggy and had > >insufficient review prior to merging. > > That's not how Linux kernel development works. If a bug can get fixed > easily, a fix is preferred instead of reverting + reapplying a patch. Your original "fix" is not right, and it wasn't properly tested. Commit 54cbc058d86beca3515c994039b5c0f0a34f53dd needs to be reverted. > >Using the "a kernel warning hit" approach for work on cancellation is > >very much a sign that the patches were half baked. > Is there perhaps a misunderstanding? My patches fix a kernel warning and > did not introduce any new WARN*() statements. The change that introduced that callback by you was incorrect and should be reverted. > >Why are you touching the kiocb after ownership has already been > >passed on to another entity? > Touching the kiocb after ownership has been passed is the result of an > oversight. Whether or not kiocb->ki_cancel() transfers ownership depends > on the I/O type. The use-after-free was not introduced on purpose. Your fix is still incorrect. You're still touching memory that you don't own. The event should be generated via the ->ki_cancel method, not in the io_cancel() syscall. -ben > Bart. > >
diff --git a/fs/aio.c b/fs/aio.c index 28223f511931..0fed22ed9eb8 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -1762,9 +1762,8 @@ static void aio_poll_complete_work(struct work_struct *work) } /* else, POLLFREE has freed the waitqueue, so we must complete */ list_del_init(&iocb->ki_list); iocb->ki_res.res = mangle_poll(mask); - spin_unlock_irq(&ctx->ctx_lock); - iocb_put(iocb); + spin_unlock_irq(&ctx->ctx_lock); } /* assumes we are called with irqs disabled */ @@ -2198,7 +2197,6 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, break; } } - spin_unlock_irq(&ctx->ctx_lock); /* * The result argument is no longer used - the io_event is always @@ -2206,6 +2204,7 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb, */ if (ret == 0 && kiocb->rw.ki_flags & IOCB_AIO_RW) aio_complete_rw(&kiocb->rw, -EINTR); + spin_unlock_irq(&ctx->ctx_lock); percpu_ref_put(&ctx->users);