Subject: Re: [PATCH 06/20] staging/lustre: fix comparison between signed and unsigned
Mime-Version: 1.0 (Apple Message framework v1283)
Content-Type: text/plain; charset=windows-1252
From: Oleg Drokin <green@linuxhacker.ru>
In-Reply-To: <20150202154400.GB18209@kroah.com>
Date: Mon, 2 Feb 2015 15:25:58 -0500
Cc: Dan Carpenter <dan.carpenter@oracle.com>, devel@driverdev.osuosl.org,
        Dmitry Eremin <dmitry.eremin@intel.com>,
        Andreas Dilger <andreas.dilger@intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <210DE60C-B152-4DF5-B6FC-06CD47CD36B9@linuxhacker.ru>
References: <1422845539-26742-1-git-send-email-green@linuxhacker.ru> <1422845539-26742-7-git-send-email-green@linuxhacker.ru> <20150202130231.GA5451@mwanda> <20150202154400.GB18209@kroah.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1862
Lines: 52

Hello!

On Feb 2, 2015, at 10:44 AM, Greg Kroah-Hartman wrote:

> On Mon, Feb 02, 2015 at 04:02:31PM +0300, Dan Carpenter wrote:
>> On Sun, Feb 01, 2015 at 09:52:05PM -0500, green@linuxhacker.ru wrote:
>>> From: Dmitry Eremin <dmitry.eremin@intel.com>
>>> 
>>> Expression if (size != (ssize_t)size) is always false.
>>> Therefore no bounds check errors detected.
>> 
>> The original code actually worked as designed.  The integer overflow
>> could only happen on 32 bit systems and the test only was true for 32
>> bit systems.

Hm, indeed.
Originally I fell into the trap thinking we are trying to protect against
negative results here too. But in fact callers all check for the return
to be negative as an error sign. Not to mention that we cannot overflow
64bit integer here as explained by the comment just 2 lines above the
default patch context.

>> 
>>> -	if (size != (ssize_t)size)
>>> +	if (size > ~((size_t)0)>>1)
>>> 		return -1;
>> 
>> The problem is that the code was unclear.  I think the new code is even
>> more complicated to look at.
> I agree, I don't even understand what the new code is doing.

Sorry, this patch indeed should be dropped.

> What is this code supposed to be protecting from?  And -1?  That should
> never be a return value?

Why is -1 a bad return value if all callsites check for that as an
indication of error?
(granted there's only one caller at this point in kernel space:
lustre/llite/dir.c::ll_dir_ioctl()
                totalsize = hur_len(hur);
                OBD_FREE_PTR(hur);
                if (totalsize < 0)
                        return -E2BIG;
)

Bye,
    Oleg--
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/