Re: [PATCH 1/6] livepatch v5: avoid race when checking for state of the patch

[Petr Mladek]

On Tue 2014-12-09 12:32:49, Josh Poimboeuf wrote:
> On Tue, Dec 09, 2014 at 07:05:02PM +0100, Petr Mladek wrote:
> > klp_patch_enable() and klp_patch_disable() should check the current state
> > of the patch under the klp_lock. Otherwise, it might detect that the operation
> > is valid but the situation might change before it takes the lock.
> 
> Hi Petr,
> 
> Thanks for the patches.
> 
> I don't think this patch is necessary.  klp_is_enabled() doesn't check
> the state of the patch.  It checks the initialization state of the core
> module (klp_root_kobj), which can only be set in klp_init().  It's not
> protected by the lock, so I don't see the point of this patch.

Ah, I have misread the name and expected that it checked whether
the patch was enabled or disabled. The original code is OK then.

Well, Jiri Kosina pointed out that the check did not make much sense.
klp_is_enabled() could not be called if the livepatch module is not
loaded. And the later check for klp_patch_is_registered() is enough
to check whether the klp_enable_patch()/klp_disable_patch() calls
are allowed or not.

So, I suggest to remove the checks at all.

Best Regards,
Petr


> > 
> > Signed-off-by: Petr Mladek <pmladek@xxxxxxx>
> > ---
> >  kernel/livepatch/core.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index d6d0f50e81f8..b848069e44cc 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -425,11 +425,13 @@ int klp_disable_patch(struct klp_patch *patch)
> >  {
> >  	int ret;
> >  
> > -	if (!klp_is_enabled())
> > -		return -ENODEV;
> > -
> >  	mutex_lock(&klp_mutex);
> >  
> > +	if (!klp_is_enabled()) {
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> >  	if (!klp_patch_is_registered(patch)) {
> >  		ret = -EINVAL;
> >  		goto err;
> > @@ -489,11 +491,13 @@ int klp_enable_patch(struct klp_patch *patch)
> >  {
> >  	int ret;
> >  
> > -	if (!klp_is_enabled())
> > -		return -ENODEV;
> > -
> >  	mutex_lock(&klp_mutex);
> >  
> > +	if (!klp_is_enabled()) {
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> >  	if (!klp_patch_is_registered(patch)) {
> >  		ret = -EINVAL;
> >  		goto err;
> > -- 
> > 1.8.5.2
> > 
> 
> -- 
> Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/