From patchwork Mon Oct 21 21:13:01 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Geyslan G. Bem" X-Patchwork-Id: 3079581 Return-Path: X-Original-To: patchwork-v9fs-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id EB7FABF924 for ; Mon, 21 Oct 2013 21:13:15 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A2B68203E9 for ; Mon, 21 Oct 2013 21:13:14 +0000 (UTC) Received: from lists.sourceforge.net (lists.sourceforge.net [216.34.181.88]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 12BC4203A8 for ; Mon, 21 Oct 2013 21:13:13 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=sfs-ml-3.v29.ch3.sourceforge.com) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1VYMmk-0003wi-Gr; Mon, 21 Oct 2013 21:13:10 +0000 Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193] helo=mx.sourceforge.net) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1VYMmh-0003vq-U3 for v9fs-developer@lists.sourceforge.net; Mon, 21 Oct 2013 21:13:08 +0000 Received-SPF: pass (sog-mx-3.v43.ch3.sourceforge.com: domain of gmail.com designates 209.85.223.170 as permitted sender) client-ip=209.85.223.170; envelope-from=geyslan@gmail.com; helo=mail-ie0-f170.google.com; Received: from mail-ie0-f170.google.com ([209.85.223.170]) by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1VYMmg-0007Y5-Tn for v9fs-developer@lists.sourceforge.net; Mon, 21 Oct 2013 21:13:07 +0000 Received: by mail-ie0-f170.google.com with SMTP id at1so154163iec.29 for ; Mon, 21 Oct 2013 14:13:01 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.43.137.9 with SMTP id im9mr35998icc.97.1382389981466; Mon, 21 Oct 2013 14:13:01 -0700 (PDT) Received: by 10.64.81.102 with HTTP; Mon, 21 Oct 2013 14:13:01 -0700 (PDT) In-Reply-To: References: <1381184340-11190-1-git-send-email-geyslan@gmail.com> <1381185204.2081.221.camel@joe-AO722> <1381191145.2081.230.camel@joe-AO722> Date: Mon, 21 Oct 2013 19:13:01 -0200 Message-ID: From: =?UTF-8?Q?Geyslan_Greg=C3=B3rio_Bem?= To: Eric Van Hensbergen X-Spam-Score: -1.6 (-) X-Headers-End: 1VYMmg-0007Y5-Tn Cc: Latchesar Ionkov , linux-kernel , V9FS Developers , Joe Perches , Ron Minnich , kernel-br Subject: Re: [V9fs-developer] [PATCH] 9p: unsigned/signed wrap in p9/unix modes. X-BeenThere: v9fs-developer@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: v9fs-developer-bounces@lists.sourceforge.net X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, FREEMAIL_FROM,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 2013/10/21 Geyslan Gregório Bem : > 2013/10/20 Eric Van Hensbergen : >> Please resubmit a clean patch which includes the check of sscanf for exactly >> the correct number of arguments and handles errors properly in other cases. >> That last bit may be a bit problematic since right now the only errors are >> prints and we seem to be otherwise silently failing. Of course, looks like >> nothing else is checking return values from that function for error. We >> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in >> all the places which matter (looks like there are a few places where rdev >> just gets discarded -- might even be a good idea to not parse rdev unless we >> need to by passing NULL to p9mode2unixmode. >> >> All in all, its a corner case which is only likely with a broken server, but >> the full clean up would seem to be: >> a) switch to u32's >> b) pass NULL when rdev just gets discarded and don't bother parsing when >> it is >> c) check the sscanf return validity >> d) on error set ERR_PTR in rdev and check on return before proceeding >> >> That's a lot of cleanup, I'll add it to my work queue if you don't have time >> to rework your patch. >> > > Eric, I would like to try with your guidance. > >> For the other patches, anyone you didn't see a response from me on today is >> being pulled into my for-next queue. Thanks for the cleanups. >> >> -eric > > Thanks for accept them. > >> >> >> >> >> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem >> wrote: >>> >>> Joe, >>> >>> Nice, I'll wait their reply, there are other p9 patches that I have >>> already sent and am awaiting Eric's. >>> >>> Thank you again. >>> >>> Geyslan Gregório Bem >>> hackingbits.com >>> Let me know if I got your requests: res |= P9_DMDIR; @@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses, static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses, struct p9_wstat *stat, dev_t *rdev) { - int res; + umode_t res; u32 mode = stat->mode; *rdev = 0; @@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses, else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses)) && (v9ses->nodev == 0)) { char type = 0, ext[32]; - int major = -1, minor = -1; + u32 major = 0, minor = 0; strlcpy(ext, stat->extension, sizeof(ext)); - sscanf(ext, "%c %u %u", &type, &major, &minor); + if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) { + p9_debug(P9_DEBUG_ERROR, + "It's necessary define type [%u], major [u%] and minor [u%]" \ + "values when using P9_DMDEVICE", type, major, minor); + goto err_dev; + } switch (type) { case 'c': res |= S_IFCHR; @@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses, default: p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n", type, stat->extension); + goto err_dev; }; *rdev = MKDEV(major, minor); } else res |= S_IFREG; + goto ret; +err_dev: + rdev = ERR_PTR(-EIO); +ret: return res; } @@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode) static int v9fs_test_inode(struct inode *inode, void *data) { - int umode; + umode_t umode; dev_t rdev; struct v9fs_inode *v9inode = V9FS_I(inode); struct p9_wstat *st = (struct p9_wstat *)data; struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode); umode = p9mode2unixmode(v9ses, st, &rdev); + + if (IS_ERR(rdev)) + return 0; + /* don't match inode of different type */ if ((inode->i_mode & S_IFMT) != (umode & S_IFMT)) return 0; @@ -526,6 +540,11 @@ static struct inode *v9fs_qid_iget(struct super_block *sb, */ inode->i_ino = i_ino; umode = p9mode2unixmode(v9ses, st, &rdev); + if (IS_ERR(rdev)) { + retval = rdev; + goto error; + } + retval = v9fs_init_inode(v9ses, inode, umode, rdev); if (retval) goto error; @@ -1461,9 +1480,10 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t rde int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode) { - int umode; + umode_t umode; dev_t rdev; loff_t i_size; + int ret = 0; struct p9_wstat *st; struct v9fs_session_info *v9ses; @@ -1475,6 +1495,11 @@ int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode) * Don't update inode if the file type is different */ umode = p9mode2unixmode(v9ses, st, &rdev); + if (IS_ERR(rdev)) { + ret = rdev; + goto out; + } + if ((inode->i_mode & S_IFMT) != (umode & S_IFMT)) goto out; @@ -1491,7 +1516,7 @@ int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode) out: p9stat_free(st); kfree(st); - return 0; + return ret; } static const struct inode_operations v9fs_dir_inode_operations_dotu = { >>> >>> 2013/10/7 Joe Perches : >>> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote: >>> >> Joe, >>> >> >>> >> Thank you for reply. >>> >> >>> >> What do you think about: >>> >> >>> >> strncpy(ext, stat->extension, sizeof(ext)); >>> >> + if (sscanf(ext, "%c %u %u", &type, &major, &minor) < >>> >> 3) { >>> >> + p9_debug(P9_DEBUG_ERROR, >>> >> + "It's necessary define type, major >>> >> and minor values when using P9_DMDEVICE"); >>> >> + return res; >>> >> + } >>> >> switch (type) { >>> >> case 'c': >>> >> res |= S_IFCHR; >>> >> break; >>> >> ... >>> >> *rdev = MKDEV(major, minor); >>> > >>> > I think the plan 9 folk should figure out what's right. >>> > >>> > >> >> ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 94de6d1..8a332d0 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -63,7 +63,7 @@ static const struct inode_operations v9fs_symlink_inode_operations; static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode) { - int res; + u32 res; res = mode & 0777; if (S_ISDIR(mode))