Re: [RFC PATCH 8/9] livepatch: allow patch modules to be removed

[Miroslav Benes <mbenes@suse.cz>]

On Tue, 17 Feb 2015, Josh Poimboeuf wrote:

> On Mon, Feb 16, 2015 at 05:06:15PM +0100, Miroslav Benes wrote:
> > On Fri, 13 Feb 2015, Josh Poimboeuf wrote:
> > 
> > > On Fri, Feb 13, 2015 at 05:17:10PM +0100, Miroslav Benes wrote:
> > > > On Fri, 13 Feb 2015, Josh Poimboeuf wrote:
> > > > > Hm, even with Jiri Slaby's suggested fix to add the completion to the
> > > > > unregister path, I still get a lockdep warning.  This looks more insidious,
> > > > > related to the locking order of a kernfs lock and the klp lock.  I'll need to
> > > > > look at this some more...
> > > > 
> > > > Yes, I was afraid of this. Lockdep warning is a separate bug. It is caused 
> > > > by taking klp_mutex in enabled_store. During rmmod klp_unregister_patch 
> > > > takes klp_mutex and destroys the sysfs structure. If somebody writes to 
> > > > enabled just after unregister takes the mutex and before the sysfs 
> > > > removal, he would cause the deadlock, because enabled_store takes the 
> > > > "sysfs lock" and then klp_mutex. That is exactly what the lockdep tells us 
> > > > below.
> > > > 
> > > > We can look for inspiration elsewhere. Grep for s_active through git log 
> > > > of the mainline offers several commits which dealt exactly with this. Will 
> > > > browse through that...
> > > 
> > > Thanks Miroslav, please let me know what you find.  It wouldn't surprise
> > > me if this were a very common problem.
> > > 
> > > One option would be to move the enabled_store() work out to a workqueue
> > > or something.
> > 
> > Yes, that is one possibility. It is not the only one.
> > 
> > 1. we could replace mutex_lock in enabled_store with mutex_trylock. If the 
> > lock was not acquired we would return -EBUSY. Or could we 'return 
> > restart_syscall' (maybe after some tiny msleep)?
> 
> Hm, doesn't that still violate the locking order rules?  I thought locks
> always had to be taken in the same order -- always sysfs before klp, or
> klp before sysfs.  Not sure if there would still be any deadlocks
> lurking, but lockdep might still complain.

Yes, but in this case you break the possible deadlock order. From the 
lockdep report...

   CPU0                    CPU1
   ----                    ----
   lock(klp_mutex);
                           lock(s_active#70);
                           lock(klp_mutex);
   lock(s_active#70);

CPU0 called klp_unregister_patch and CPU1 possible enabled_store in a race 
window. Deadlock wouldn't be there because trylock(klp_mutex) on CPU1 
would return 0 and enabled_store thus EBUSY. And in every other scenario 
trylock would prevent deadlock too or klp_unregister_patch would wait on 
klp_mutex (I hope I did not miss anything).

I tried it and lockdep did not complain. 

And you can look at 36c38fb7144aa941dc072ba8f58b2dbe509c0345 or 
5e33bc4165f3edd558d9633002465a95230effc1. They dealt with it the same way 
(but it does not mean anything).

It would need more testing to be sure though.

> > 2. we could reorganize klp_unregister_patch somehow and move sysfs removal 
> > out of mutex protection.
> 
> Yeah, I was thinking about this too.  Pretty sure we'd have to remove
> both the sysfs add and the sysfs removal from mutex protection.  I like
> this option if we can get it to work.

Yes, why not.

Maybe someone else will share his opinion on this...

Miroslav