From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933537AbcKMLDZ (ORCPT ); Sun, 13 Nov 2016 06:03:25 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:35442 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933121AbcKMLDX (ORCPT ); Sun, 13 Nov 2016 06:03:23 -0500 Date: Sun, 13 Nov 2016 12:03:34 +0100 From: Greg KH To: Peter Zijlstra Cc: Kees Cook , "kernel-hardening@lists.openwall.com" , Will Deacon , Elena Reshetova , Arnd Bergmann , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , LKML Subject: Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC Message-ID: <20161113110334.GF8388@kroah.com> References: <1478809488-18303-1-git-send-email-elena.reshetova@intel.com> <20161110203749.GV3117@twins.programming.kicks-ass.net> <20161110204838.GE17134@arm.com> <20161110211310.GX3117@twins.programming.kicks-ass.net> <20161110222744.GD8086@kroah.com> <20161110235714.GR3568@worktop.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161110235714.GR3568@worktop.programming.kicks-ass.net> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Nov 11, 2016 at 12:57:14AM +0100, Peter Zijlstra wrote: > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote: > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH wrote: > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote: > > > >> As it stands kref is a pointless wrapper. If it were to provide > > >> something actually useful, like wrap protection, then it might actually > > >> make sense to use it. > > > > > > It provides the correct cleanup ability for a reference count and the > > > object it is in, so it's not all that pointless :) > > I really fail to see the point of: > > kref_put(&obj->ref, obj_free); > > over: > > if (atomic_dec_and_test(&obj->ref)) > obj_free(obj); > > Utter pointless wrappery afaict. No, it's an abstraction that shows what you are trying to have the driver code do (drop a reference), and is simple to track and verify that the driver is doing stuff properly (automated tools can do it too). It's also easier to review and audit, if I see a developer use a kref, I know how to easily read the code to verify it works correctly. If they use a "raw" atomic, I need to spend more time and effort to review that they got it all correctly (always test the same way, call the correct function the same way, handle the lock that needs to be somewhere involved here, etc.) So it saves both new developers time and effort (how do I write a reference count properly...) and experienced developers (easy to audit and read), which is why we have kref in the first place. I'm all for having it use a wrap-free type, if possible, to catch these types of errors, and yes, you are right in that it would only take 3 functions to implement it correctly. I'd love to have it, but please, don't say that krefs are pointless, they aren't. thanks, greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com Date: Sun, 13 Nov 2016 12:03:34 +0100 From: Greg KH Message-ID: <20161113110334.GF8388@kroah.com> References: <1478809488-18303-1-git-send-email-elena.reshetova@intel.com> <20161110203749.GV3117@twins.programming.kicks-ass.net> <20161110204838.GE17134@arm.com> <20161110211310.GX3117@twins.programming.kicks-ass.net> <20161110222744.GD8086@kroah.com> <20161110235714.GR3568@worktop.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161110235714.GR3568@worktop.programming.kicks-ass.net> Subject: Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC To: Peter Zijlstra Cc: Kees Cook , "kernel-hardening@lists.openwall.com" , Will Deacon , Elena Reshetova , Arnd Bergmann , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , LKML List-ID: On Fri, Nov 11, 2016 at 12:57:14AM +0100, Peter Zijlstra wrote: > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote: > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH wrote: > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote: > > > >> As it stands kref is a pointless wrapper. If it were to provide > > >> something actually useful, like wrap protection, then it might actually > > >> make sense to use it. > > > > > > It provides the correct cleanup ability for a reference count and the > > > object it is in, so it's not all that pointless :) > > I really fail to see the point of: > > kref_put(&obj->ref, obj_free); > > over: > > if (atomic_dec_and_test(&obj->ref)) > obj_free(obj); > > Utter pointless wrappery afaict. No, it's an abstraction that shows what you are trying to have the driver code do (drop a reference), and is simple to track and verify that the driver is doing stuff properly (automated tools can do it too). It's also easier to review and audit, if I see a developer use a kref, I know how to easily read the code to verify it works correctly. If they use a "raw" atomic, I need to spend more time and effort to review that they got it all correctly (always test the same way, call the correct function the same way, handle the lock that needs to be somewhere involved here, etc.) So it saves both new developers time and effort (how do I write a reference count properly...) and experienced developers (easy to audit and read), which is why we have kref in the first place. I'm all for having it use a wrap-free type, if possible, to catch these types of errors, and yes, you are right in that it would only take 3 functions to implement it correctly. I'd love to have it, but please, don't say that krefs are pointless, they aren't. thanks, greg k-h