From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753572AbdHKQfY (ORCPT ); Fri, 11 Aug 2017 12:35:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59166 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753526AbdHKQfV (ORCPT ); Fri, 11 Aug 2017 12:35:21 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E99D67ACAE Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jpoimboe@redhat.com Date: Fri, 11 Aug 2017 11:35:11 -0500 From: Josh Poimboeuf To: Joe Lawrence Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu , Jiri Kosina , Miroslav Benes , Petr Mladek Subject: Re: [PATCH v3] livepatch: introduce shadow variable API Message-ID: <20170811163511.psyran7h2zvsxtfk@treble> References: <1501262722-26502-1-git-send-email-joe.lawrence@redhat.com> <1501262722-26502-2-git-send-email-joe.lawrence@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1501262722-26502-2-git-send-email-joe.lawrence@redhat.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 11 Aug 2017 16:35:21 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 28, 2017 at 01:25:22PM -0400, Joe Lawrence wrote: > Add exported API for livepatch modules: > > klp_shadow_get() > klp_shadow_attach() > klp_shadow_get_or_attach() > klp_shadow_update_or_attach() > klp_shadow_detach() > klp_shadow_detach_all() I like the API. > +Matching parent's lifecycle > +--------------------------- > + > +If parent data structures are frequently created and destroyed, it may > +be easiest to align its shadow variable lifetimes to the same allocation "its shadow variable lifetimes" -> "their shadow variables' lifetimes" ? > +void *klp_shadow_attach(void *obj, unsigned long id, void *data, > + size_t size, gfp_t gfp_flags) [ Note: some of the following comments also apply to klp_shadow_get_or_attach and klp_shadow_update_or_attach. ] > +{ > + struct klp_shadow *new_shadow; nit: The "new" is implied, and there's only one shadow struct used in the function, so maybe just call it "shadow"? > + void *shadow_data; > + unsigned long flags; > + > + /* Take error exit path if already exists */ > + shadow_data = klp_shadow_get(obj, id); > + if (unlikely(shadow_data)) > + goto err_exists; The return value of klp_shadow_get() can be tested directly, no need for a variable. > + /* Allocate a new shadow variable for use inside the lock below */ > + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags); > + if (!new_shadow) > + goto err; The goto destination is just a single line return, so I think it's clearer to just return NULL here and get rid of the err label. > + > + /* Look for again under the lock */ > + spin_lock_irqsave(&klp_shadow_lock, flags); > + shadow_data = klp_shadow_get(obj, id); > + if (unlikely(shadow_data)) { > + > + /* Shadow variable found, throw away allocation and take > + * error exit path */ Multi-line comments should be in the kernel coding style: /* * Shadow variable found, throw away allocation and take * error exit path */ Also, complete sentences preferred :-) > + spin_unlock_irqrestore(&klp_shadow_lock, flags); > + kfree(shadow_data); Shouldn't it free new_shadow instead of shadow_data? > + goto err_exists; > + } > + > + /* No found, add the newly allocated one */ > + shadow_data = data; > + klp_shadow_set(new_shadow, obj, id, data, size); To avoid doing extra work with the lock held, the klp_shadow_set() can be done before getting the lock, after the kzalloc. -- Josh