Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 11 Jun 2018 14:43:01 +0200 (CEST)
From:   Miroslav Benes <mbenes@suse.cz>
To:     Petr Mladek <pmladek@suse.com>
cc:     jikos@kernel.org, jpoimboe@redhat.com, jeyu@kernel.org,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] livepatch: Deny the patched modules to be removed
In-Reply-To: <20180611114121.2u7wt6d6xu2cm6it@pathway.suse.cz>
Message-ID: <alpine.LSU.2.21.1806111429580.1279@pobox.suse.cz>
References: <20180607092949.1706-1-mbenes@suse.cz> <20180607092949.1706-3-mbenes@suse.cz> <20180611114121.2u7wt6d6xu2cm6it@pathway.suse.cz>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 11 Jun 2018, Petr Mladek wrote:

> On Thu 2018-06-07 11:29:48, Miroslav Benes wrote:
> > We decided to deny the patched modules to be removed. If it proves to be
> > a major drawback for users, we can still implement a different approach.
> > 
> > The reference of a patched module has to be taken regardless of a
> > patch's state. Thus it is not taken and dropped in enable/disable paths,
> > but in register/unregister paths.
> 
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -739,6 +746,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Do not allow patched modules to be removed.
> > +	 *
> > +	 * All callers of klp_init_object_loaded() set obj->mod.
> > +	 */
> > +	if (klp_is_module(obj) && !try_module_get(obj->mod))
> > +		return -ENODEV;
> > +
> >  	return 0;
> >  }
> 
> This looks strange. We try to take the reference after we do all
> actions needed for a loaded module. There is nothing that would
> prevent obj->mod to get into the going state.

Yes, there was an intention to keep the number of necessary modifications 
of the error paths as low as possible.
 
> Note that it was safe (except for the relocated symbols) because
> the module was not able to really go until it called
> klp_module_going(). But you removed this last break
> by the 3rd patch.

Yes, try_module_get() would return an error here.
 
> I suggest another approach:
> 
> I would rename klp_find_object_module() to klp_get_object_module()
> and get the reference there. Note that it should be fine to call
> it later in klp_init_object() (before klp_init_object_loaded()).
> This would solve klp_init_patch().
> 
> We will also need to get the reference in klp_module_coming().
> It can be called anywhere there. If it fails, we will block
> loading the module.

I can as well move the above hunk up in klp_init_object_loaded(), no? Not 
that I'm seeing a problem to have it in klp_find_object_module().
 
> Note that the mod->klp_alive logic will still be necessary.
> It solves various races when both the patch and the patched
> module are loaded in parallel.
>
> Sigh, this also means that we still could call klp_init_object_loaded()
> and apply reloacations even when the patched module fails to loaded
> later from other reasons. This means that this approach does not
> solve the original problem completely.

That would be in klp_module_coming(). Hm, you're right.
 
> I wonder how complicated would be the arch-specific part that
> would clean up relocations when the module is unloaded.

I don't think it would be complicated. We just want to avoid it, because 
it is arch-specific. It is more difficult to maintain such code.

Regards,
Miroslav