Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938318AbcKPKTG (ORCPT ); Wed, 16 Nov 2016 05:19:06 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:51528 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932414AbcKPKTF (ORCPT ); Wed, 16 Nov 2016 05:19:05 -0500 Date: Wed, 16 Nov 2016 11:19:15 +0100 From: Greg KH To: Daniel Borkmann Cc: Kees Cook , Peter Zijlstra , Will Deacon , "Reshetova, Elena" , Arnd Bergmann , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , David Windsor , LKML , Alexei Starovoitov Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read() Message-ID: <20161116101915.GB23902@kroah.com> References: <20161114173946.501528675@infradead.org> <20161114174446.486581399@infradead.org> <20161115073322.GC28248@kroah.com> <20161115080314.GD3142@twins.programming.kicks-ass.net> <20161116082151.GA24017@kroah.com> <582C30DF.60205@iogearbox.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <582C30DF.60205@iogearbox.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 Content-Length: 3300 Lines: 69 On Wed, Nov 16, 2016 at 11:11:43AM +0100, Daniel Borkmann wrote: > On 11/16/2016 09:21 AM, Greg KH wrote: > > On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote: > > > On Tue, Nov 15, 2016 at 12:03 AM, Peter Zijlstra wrote: > > > > On Tue, Nov 15, 2016 at 08:33:22AM +0100, Greg KH wrote: > > > > > On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote: > > > > > > > > > > --- a/drivers/block/drbd/drbd_req.c > > > > > > +++ b/drivers/block/drbd/drbd_req.c > > > > > > @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req > > > > > > /* Completion does it's own kref_put. If we are going to > > > > > > * kref_sub below, we need req to be still around then. */ > > > > > > int at_least = k_put + !!c_put; > > > > > > - int refcount = atomic_read(&req->kref.refcount); > > > > > > + int refcount = kref_read(&req->kref); > > > > > > if (refcount < at_least) > > > > > > drbd_err(device, > > > > > > "mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n", > > > > > > > > > > As proof of "things you should never do", here is one such example. > > > > > > > > > > ugh. > > > > > > > > > > > --- a/drivers/block/virtio_blk.c > > > > > > +++ b/drivers/block/virtio_blk.c > > > > > > @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio > > > > > > /* Stop all the virtqueues. */ > > > > > > vdev->config->reset(vdev); > > > > > > > > > > > > - refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount); > > > > > > + refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref); > > > > > > put_disk(vblk->disk); > > > > > > vdev->config->del_vqs(vdev); > > > > > > kfree(vblk->vqs); > > > > > > > > > > And this too, ugh, that's a huge abuse and is probably totally wrong... > > > > > > > > > > thanks again for digging through this crap. I wonder if we need to name > > > > > the kref reference variable "do_not_touch_this_ever" or some such thing > > > > > to catch all of the people who try to be "too smart". > > > > > > > > There's unimaginable bong hits involved in this stuff, in the end I > > > > resorted to brute force and scripts to convert all this. > > > > > > What should we do about things like this (bpf_prog_put() and callbacks > > > from kernel/bpf/syscall.c): > > Just reading up on this series. Your question refers to converting bpf > prog and map ref counts to Peter's refcount_t eventually, right? > > > > static void bpf_prog_uncharge_memlock(struct bpf_prog *prog) > > > { > > > struct user_struct *user = prog->aux->user; > > > > > > atomic_long_sub(prog->pages, &user->locked_vm); > > > > Oh that's scary. Let's just make one reference count rely on another > > one and not check things... > > Sorry, could you elaborate what you mean by 'check things', you mean for > wrap around? IIUC, back then accounting was roughly similar modeled after > perf event's one, and in this case accounts for pages used by progs and > maps during their life-time. Are you suggesting that this approach is > inherently broken? No, it is correct, I responded too quickly before my morning coffee had kicked in, my apologies. greg k-h