From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965104AbbBBUvp (ORCPT ); Mon, 2 Feb 2015 15:51:45 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:33612 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753260AbbBBUvo (ORCPT ); Mon, 2 Feb 2015 15:51:44 -0500 Date: Mon, 2 Feb 2015 12:51:43 -0800 From: Greg Kroah-Hartman To: Oleg Drokin Cc: Dan Carpenter , devel@driverdev.osuosl.org, Dmitry Eremin , Andreas Dilger , Linux Kernel Mailing List Subject: Re: [PATCH 06/20] staging/lustre: fix comparison between signed and unsigned Message-ID: <20150202205143.GA6369@kroah.com> 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> <210DE60C-B152-4DF5-B6FC-06CD47CD36B9@linuxhacker.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <210DE60C-B152-4DF5-B6FC-06CD47CD36B9@linuxhacker.ru> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 02, 2015 at 03:25:58PM -0500, Oleg Drokin wrote: > 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 > >>> > >>> 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? Because you should use "real" error values, don't make them up with random negative numbers that mean nothing. > (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; > ) Shouldn't you have returned the error that hur_len() passed you? thanks, greg k-h