Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760285Ab3GSM0Q (ORCPT ); Fri, 19 Jul 2013 08:26:16 -0400 Received: from mail9.hitachi.co.jp ([133.145.228.44]:52467 "EHLO mail9.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751149Ab3GSM0O (ORCPT ); Fri, 19 Jul 2013 08:26:14 -0400 Message-ID: <51E93062.4090405@hitachi.com> Date: Fri, 19 Jul 2013 21:26:10 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 5.2; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: Yoshihiro YUNOMAE Cc: Amit Shah , linux-kernel@vger.kernel.org, Arnd Bergmann , Greg Kroah-Hartman , stable@vger.kernel.org, virtualization@lists.linux-foundation.org, Hidehiro Kawai , yrl.pp-manager.tt@hitachi.com Subject: Re: [PATCH V2 2/2] [BUGFIX] virtio/console: Add pipe_lock/unlock for splice_write References: <20130719091951.9698.95194.stgit@yunodevel> <20130719091956.9698.30207.stgit@yunodevel> In-Reply-To: <20130719091956.9698.30207.stgit@yunodevel> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6214 Lines: 151 (2013/07/19 18:19), Yoshihiro YUNOMAE wrote: > Add pipe_lock/unlock for splice_write to avoid oops by following competition: > > (1) An application gets fds of a trace buffer, virtio-serial, pipe. > (2) The application does fork() > (3) The processes execute splice_read(trace buffer) and > splice_write(virtio-serial) via same pipe. > > > get fds of a trace buffer, > virtio-serial, pipe > | > fork()----------create--------+ > | | > splice(read) | ---+ > splice(write) | +-- no competition > | splice(read) | > | splice(write) ---+ > | | > splice(read) | > splice(write) splice(read) ------ competition > | splice(write) > > Two processes share a pipe_inode_info structure. If the child execute > splice(read) when the parent tries to execute splice(write), the > structure can be broken. Existing virtio-serial driver does not get > lock for the structure in splice_write, so this competition will induce > oops. > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000018 > IP: [] splice_from_pipe_feed+0x6f/0x130 > PGD 7223e067 PUD 72391067 PMD 0 > Oops: 0000 [#1] SMP > Modules linked in: lockd bnep bluetooth rfkill sunrpc ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_timer snd soundcore pcspkr virtio_net virtio_balloon i2c_piix4 i2c_core microcode uinput floppy > CPU: 0 PID: 1072 Comm: compete-test Not tainted 3.10.0ws+ #55 > Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 > task: ffff880071b98000 ti: ffff88007b55e000 task.ti: ffff88007b55e000 > RIP: 0010:[] [] splice_from_pipe_feed+0x6f/0x130 > RSP: 0018:ffff88007b55fd78 EFLAGS: 00010287 > RAX: 0000000000000000 RBX: ffff88007b55fe20 RCX: 0000000000000000 > RDX: 0000000000001000 RSI: ffff88007a95ba30 RDI: ffff880036f9e6c0 > RBP: ffff88007b55fda8 R08: 00000000000006ec R09: ffff880077626708 > R10: 0000000000000003 R11: ffffffff8139ca59 R12: ffff88007a95ba30 > R13: 0000000000000000 R14: ffffffff8139dd00 R15: ffff880036f9e6c0 > FS: 00007f2e2e3a0740(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > CR2: 0000000000000018 CR3: 0000000071bd1000 CR4: 00000000000006f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > Stack: > ffffffff8139ca59 ffff88007b55fe20 ffff880036f9e6c0 ffffffff8139dd00 > ffff8800776266c0 ffff880077626708 ffff88007b55fde8 ffffffff811a6e8e > ffff88007b55fde8 ffffffff8139ca59 ffff880036f9e6c0 ffff88007b55fe20 > Call Trace: > [] ? alloc_buf.isra.13+0x39/0xb0 > [] ? virtcons_restore+0x100/0x100 > [] __splice_from_pipe+0x7e/0x90 > [] ? alloc_buf.isra.13+0x39/0xb0 > [] port_fops_splice_write+0xe9/0x140 > [] ? selinux_file_permission+0xc4/0x120 > [] ? wait_port_writable+0x1b0/0x1b0 > [] do_splice_from+0xa0/0x110 > [] SyS_splice+0x5ff/0x6b0 > [] tracesys+0xdd/0xe2 > Code: 49 8b 87 80 00 00 00 4c 8d 24 d0 8b 53 04 41 8b 44 24 0c 4d 8b 6c 24 10 39 d0 89 03 76 02 89 13 49 8b 44 24 10 4c 89 e6 4c 89 ff 50 18 85 c0 0f 85 aa 00 00 00 48 89 da 4c 89 e6 4c 89 ff 41 > RIP [] splice_from_pipe_feed+0x6f/0x130 > RSP > CR2: 0000000000000018 > ---[ end trace 24572beb7764de59 ]--- > > V2: Fix a locking problem for error Thanks, this looks good for me. Reviewed-by: Masami Hiramatsu > > Signed-off-by: Yoshihiro YUNOMAE > Cc: Amit Shah > Cc: Arnd Bergmann > Cc: Greg Kroah-Hartman > --- > drivers/char/virtio_console.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 8722656..8a15af3 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -936,16 +936,21 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe, > * pipe->nrbufs == 0 means there are no data to transfer, > * so this returns just 0 for no data. > */ > - if (!pipe->nrbufs) > - return 0; > + pipe_lock(pipe); > + if (!pipe->nrbufs) { > + ret = 0; > + goto error_out; > + } > > ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK); > if (ret < 0) > - return ret; > + goto error_out; > > buf = alloc_buf(port->out_vq, 0, pipe->nrbufs); > - if (!buf) > - return -ENOMEM; > + if (!buf) { > + ret = -ENOMEM; > + goto error_out; > + } > > sgl.n = 0; > sgl.len = 0; > @@ -953,12 +958,17 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe, > sgl.sg = buf->sg; > sg_init_table(sgl.sg, sgl.size); > ret = __splice_from_pipe(pipe, &sd, pipe_to_sg); > + pipe_unlock(pipe); > if (likely(ret > 0)) > ret = __send_to_port(port, buf->sg, sgl.n, sgl.len, buf, true); > > if (unlikely(ret <= 0)) > free_buf(buf, true); > return ret; > + > +error_out: > + pipe_unlock(pipe); > + return ret; > } > > static unsigned int port_fops_poll(struct file *filp, poll_table *wait) > > -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/