DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E4DB912033
Date: Thu, 10 Aug 2017 12:04:32 -0400
From: Joe Lawrence <joe.lawrence@redhat.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH v3] livepatch: introduce shadow variable API
Message-ID: <20170810160432.3vccq4clvg7pi7m4@redhat.com>
References: <1501262722-26502-1-git-send-email-joe.lawrence@redhat.com>
 <1501262722-26502-2-git-send-email-joe.lawrence@redhat.com>
 <alpine.LSU.2.20.1708101631080.7971@pobox.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LSU.2.20.1708101631080.7971@pobox.suse.cz>
User-Agent: Mutt/1.6.2-neo (2016-08-08)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2488
Lines: 84

On Thu, Aug 10, 2017 at 04:40:05PM +0200, Miroslav Benes wrote:
> 
> It generally looks ok. Only few questions below...
> 
> [...]
> 
> > +In-flight parent objects
> > +------------------------
> > +	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> > +			&ps_lock_fallback, sizeof(ps_lock_fallback),
> > +			GFP_ATOMIC);
> > +
> > +	ps_lock = klp_shadow_get(sta, PS_LOCK);
> > +	if (ps_lock)
> > +		spin_lock(ps_lock);
> 
> ps_lock = klp_shadow_get(sta, PS_LOCK); should not be needed, should it?

Correct, I'll remove it.

> [...]
> 
> > +/*
> > + * klp_shadow_set() - initialize a shadow variable
> > + * @shadow:	shadow variable to initialize
> > + * @obj:	pointer to parent object
> > + * @id:		data identifier
> > + * @data:	pointer to data to attach to parent
> > + * @size:	size of attached data
> > + */
> > +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> > +				  unsigned long id, void *data, size_t size)
> > +{
> > +	shadow->obj = obj;
> > +	shadow->id = id;
> > +
> > +	if (data)
> > +		memcpy(shadow->data, data, size);
> > +}
> > +
> > +/**
> > + * klp_shadow_add() - add a shadow variable to the hashtable
> > + * @shadow:	shadow variable to add
> > + */
> > +static inline void klp_shadow_add(struct klp_shadow *shadow)
> > +{
> > +	hash_add_rcu(klp_shadow_hash, &shadow->node,
> > +		     (unsigned long)shadow->obj);
> > +}
> 
> It would be nice to add a comment that a caller must hold klp_shadow_lock 
> spinlock.

Since shadow_match(), klp_shadow_set(), and klp_shadow_add() are all in
the same boat, I can mention the lock for those functions as well.  BTW,
is there a convention to drop the "klp_" for static, local routines?  I
should be consistent here.
 
> > +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> > +			size_t size, gfp_t gfp_flags)
> > +	return shadow_data;
> 
> I may be missing something, but shouldn't this return new_shadow->data? 
> You return original data here which seems strange.
> 
> > +void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> > +			       size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Again. "shadow_data = new_shadow->data;"?
> 
> > +void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> > +				  size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Dtto.
> 

Gah!  Thank you for spotting this!  It was leftover from v1 when there
were only pointers (and not data copies) being thrown about.

Thanks,

-- Joe