linux-kernel - Re: [PATCH] ceph: match wait_for_completion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150311110410.GA21968@opentech.at>
Date:	Wed, 11 Mar 2015 12:04:10 +0100
From:	Nicholas Mc Guire <der.herr@...r.at>
To:	"Yan, Zheng" <ukernel@...il.com>
Cc:	Nicholas Mc Guire <hofrat@...dl.org>, Yan Zheng <zyan@...hat.com>,
	Sage Weil <sage@...hat.com>,
	ceph-devel <ceph-devel@...r.kernel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ceph: match wait_for_completion_timeout return type

On Wed, 11 Mar 2015, Yan, Zheng wrote:

> On Tue, Mar 10, 2015 at 11:18 PM, Nicholas Mc Guire <hofrat@...dl.org> wrote:
> > return type of wait_for_completion_timeout is unsigned long not int. An
> > appropriately named unsigned long is added and the assignment fixed up.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> > ---
> >
> > This was only compile tested for x86_64_defconfig + CONFIG_CEPH_FS=m
> >
> > Patch is against 4.0-rc2 linux-next (localversion-next is -next-20150306)
> >
> >  fs/ceph/dir.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 83e9976..4bee6b7 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -1218,6 +1218,7 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
> >         struct ceph_mds_request *req;
> >         u64 last_tid;
> >         int ret = 0;
> > +       unsigned long time_left;
> >
> >         dout("dir_fsync %p\n", inode);
> >         ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > @@ -1240,11 +1241,11 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
> >                 dout("dir_fsync %p wait on tid %llu (until %llu)\n",
> >                      inode, req->r_tid, last_tid);
> >                 if (req->r_timeout) {
> > -                       ret = wait_for_completion_timeout(
> > +                       time_left = wait_for_completion_timeout(
> >                                 &req->r_safe_completion, req->r_timeout);
> > -                       if (ret > 0)
> > +                       if (time_left > 0)
> >                                 ret = 0;
> > -                       else if (ret == 0)
> > +                       else if (!time_left)
> >                                 ret = -EIO;  /* timed out */
> >                 } else {
> >                         wait_for_completion(&req->r_safe_completion);
> 
> There are lots of similar codes in kernel. I don't think this code
> causes problem in reality
>
true - there are 38 (of the initial 81 files) left for which no patch has been 
submitted yet - its cleanup in progress.

type correctness I do believe is an issue and code readability as well
so both fixing the type and that name is relevant.

As Wolfram Sang <wsa@...-dreams.de> put it:
<snip>
 'ret' being an int is kind of an idiom, so I'd rather see the variable
 renamed, too, like the other patches do.
<snip>
[http://lkml.iu.edu/hypermail/linux/kernel/1502.1/00084.html]

regarding causing problems - it is hard to say - type missmatch
may go without problems for a long time and then pop up in strange
corner cases. But you are right that it is not fixing any currently
inown incorrect behavior.

The motivation for cleaning this up is also to make static code checkers
happy which eases scanning for incorrect API usage and general bug-hunting,

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/