From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751768AbdI1IC3 (ORCPT ); Thu, 28 Sep 2017 04:02:29 -0400 Received: from mx2.suse.de ([195.135.220.15]:40791 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750943AbdI1IC1 (ORCPT ); Thu, 28 Sep 2017 04:02:27 -0400 Date: Thu, 28 Sep 2017 10:02:23 +0200 From: Jean Delvare To: Ingo Molnar Cc: LKML , Andrew Morton , Baoquan He , Linus Torvalds , Thomas Gleixner , Peter Zijlstra , "H. Peter Anvin" , Borislav Petkov Subject: Re: [PATCH] params: Fix an overflow in param_attr_show Message-ID: <20170928100223.2be62f88@endymion> In-Reply-To: <20170927133104.s6meugnrysccwrde@gmail.com> References: <20170927101031.7a3b2398@endymion> <20170927082642.slh2gk3zuw5j7gmh@gmail.com> <1506505230.14970.22.camel@suse.de> <20170927133104.s6meugnrysccwrde@gmail.com> 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=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 27 Sep 2017 15:31:04 +0200, Ingo Molnar wrote: > * Jean Delvare wrote: > > > So the \n additions to the STANDARD_PARAM_DEF() lines > > > > > > > +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8); > > > > +STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16); > > > > +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16); > > > > > > are not necessary anymore, with the other changes? If so then I'd leave them > > > without the \n - that's also easier to read. > > > > What other changes are you referring to? I'm confused. Are you sure you > > read the patch entirely before commenting on it? > > I was referring to the rest of the patch, which avoids the overflow even if the \n > is not present in the pattern. You make things sound complex, when my patch is so simple. I'm simply changing the point at which the trailing \n is added. The \n must be present in the pattern, so "even if the \n is not present in the pattern" was out of scope. And it turns out that it doesn't matter at all anyway. > (...) > So what I was asking, what happens if someone adds a new entry and > forgets the \n? > > This is not hypothetical - for example this commit: > > b4210b810e50 ("Add module param type 'ullong'") > > ... added a new entry for a new param type. It's entirely possible for > new additions to happen here. > (...) > At minimum I'd suggest aligning the definitions vertically, to make sure > any missing \n stands out more, visually: > > STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8); > STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16); > STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16); > STANDARD_PARAM_DEF(int, int, "%i\n", kstrtoint); > STANDARD_PARAM_DEF(uint, unsigned int, "%u\n", kstrtouint); > STANDARD_PARAM_DEF(long, long, "%li\n", kstrtol); > STANDARD_PARAM_DEF(ulong, unsigned long, "%lu\n", kstrtoul); > STANDARD_PARAM_DEF(ullong, unsigned long long, "%llu\n", kstrtoull); Sure it is possible to add a new parameter type. But why would the person adding it forget the \n? I can't imagine that someone adding a new type would type the new line of code character by character. Such an operation is calling for copy, paste and edit, at which point there is no reason why the \n would be actively deleted. Or this is sabotage, really ;-) Aligning parameters vertically as you suggest above is probably a good idea for overall readability anyway, so I can change my patch to do that, as I am modifying these lines anyway. It is pretty much independent from the fix per se, but if it makes you happy... -- Jean Delvare SUSE L3 Support