From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751231AbdFCVO2 convert rfc822-to-8bit (ORCPT ); Sat, 3 Jun 2017 17:14:28 -0400 Received: from mx2.suse.de ([195.135.220.15]:41385 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750991AbdFCVO1 (ORCPT ); Sat, 3 Jun 2017 17:14:27 -0400 Date: Sat, 3 Jun 2017 23:14:23 +0200 From: Jean Delvare To: Andy Shevchenko Cc: LKML , Dmitry Torokhov , Mika Westerberg , Linus Walleij Subject: Re: [PATCH] firmware: dmi: Check DMI structure length Message-ID: <20170603231423.388f70b2@endymion> In-Reply-To: References: <20170601150839.08fdb556@endymion> <20170601164019.0a4035a4@endymion> <20170602204007.100d9d34@endymion> Organization: SUSE Linux X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.31; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2 Jun 2017 21:45:37 +0300, Andy Shevchenko wrote: > On Fri, Jun 2, 2017 at 9:40 PM, Jean Delvare wrote: > > On Thu, 1 Jun 2017 19:06:36 +0300, Andy Shevchenko wrote: > >> Your commit message should answer to the question why and what. > >> You didn't put it there. > >> Moreover, the change above per se doesn't belong to this — one logical > >> change per patch. > > > > I'm confused. These changes totally belong to this patch. They belong > > so much to it, that's the very reason why they are not described > > separately in the commit message. > > > > The purpose of the patch is to check that the records are large enough > > to contain the fields we need to access. Setting a pointer beyond the > > end of the record _before_ performing that check makes no sense. > > > > I did not include these changes as performance optimizations, I > > included them because they make the code conceptually correct. It's > > even clearer for the last instance, as we are dereferencing the pointer > > immediately, but in my opinion, even setting a pointer to a location > > which may not exist is equally wrong and confusing for the reader. > > That's why I moved that code after the length checks. > > You are talking here explicitly about third case which I agreed on. > > The two first ones are not the same. > You didn't dereference them before check since your check is not > against pointer. > > So, basically it means you are checking pointer _indirectly_. Correct. > I think we already spent too much time on this one. Agreed. > If you wish to leave your changes, update commit message accordingly. No. -- Jean Delvare SUSE L3 Support