All of lore.kernel.org
 help / color / mirror / Atom feed
From: Minfei Huang <mhuang@redhat.com>
To: Petr Mladek <pmladek@suse.cz>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
	Minfei Huang <mnfhuang@gmail.com>,
	mbenes@suse.cz, sjenning@redhat.com, jkosina@suse.cz,
	vojtech@suse.cz, live-patching@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails
Date: Mon, 18 May 2015 21:00:57 +0800	[thread overview]
Message-ID: <20150518130057.GA25260@dhcp-128-1.nay.redhat.com> (raw)
In-Reply-To: <20150518120806.GC2632@pathway.suse.cz>

On 05/18/15 at 02:08pm, Petr Mladek wrote:
> On Wed 2015-05-13 09:14:15, Josh Poimboeuf wrote:
> > On Tue, May 12, 2015 at 10:04:44PM +0800, Minfei Huang wrote:
> > > @@ -883,7 +883,7 @@ int klp_register_patch(struct klp_patch *patch)
> > >  }
> > >  EXPORT_SYMBOL_GPL(klp_register_patch);
> > >  
> > > -static void klp_module_notify_coming(struct klp_patch *patch,
> > > +static int klp_module_notify_coming(struct klp_patch *patch,
> > >  				     struct klp_object *obj)
> > >  {
> > >  	struct module *pmod = patch->mod;
> > > @@ -891,22 +891,24 @@ static void klp_module_notify_coming(struct klp_patch *patch,
> > >  	int ret;
> > >  
> > >  	ret = klp_init_object_loaded(patch, obj);
> > > -	if (ret)
> > > -		goto err;
> > > +	if (ret) {
> > > +		pr_warn("failed to initialize the patch '%s' (%d)\n",
> > > +				pmod->name, ret);
> > > +		goto out;
> > > +	}
> > 
> > Can you change it to:
> > 
> > "failed to initialize the patch '%s' for module '%s' (%d)\n" ?
> > 
> > That would make it more consistent with the other error message and
> > identify the failing module.
> > 
> > Also, the indentation should be fixed on the second pr_warn() line.
> > 
> > >  
> > >  	if (patch->state == KLP_DISABLED)
> > > -		return;
> > > +		goto out;
> > >  
> > >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> > >  		  pmod->name, mod->name);
> > >  
> > >  	ret = klp_enable_object(obj);
> > > -	if (!ret)
> > > -		return;
> > > -
> > > -err:
> > > -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > -		pmod->name, mod->name, ret);
> > > +	if (ret)
> > > +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > +				pmod->name, mod->name, ret);
> > 
> > Bad indentation here too.
> > 
> > > @@ -930,6 +932,7 @@ disabled:
> > >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  			     void *data)
> > >  {
> > > +	int ret;
> > >  	struct module *mod = data;
> > >  	struct klp_patch *patch;
> > >  	struct klp_object *obj;
> > > @@ -955,7 +958,13 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > >  
> > >  			if (action == MODULE_STATE_COMING) {
> > >  				obj->mod = mod;
> > > -				klp_module_notify_coming(patch, obj);
> > > +				ret = klp_module_notify_coming(patch, obj);
> > > +				if (ret) {
> > > +					obj->mod = NULL;
> > > +					pr_warn("patch '%s' is dead, remove it "
> > > +						"or re-install the module '%s'\n",
> > > +						patch->mod->name, obj->name);
> > > +				}
> > 
> > The patch isn't necessarily dead, since it might also include previously
> > enabled changes for vmlinux or other modules.  It can actually be a
> > dangerous condition if there's a mismatch between old code in the module
> > and new code elsewhere.  How about something like:
> > 
> > "patch '%s' is in an inconsistent state!\n"
> 
> It must not be dangerous, otherwise the patch could not get applied
> immediately.

But kernel is in dangerous situation that the patch may corrupt it
later. So it is appropriate to notify the user.

> 
> I would omit this message completely. It would just duplicate the
> warning printed by klp_module_notify_coming().
> 

This error message aims to tell this fact that this patch is in an
inconsistent state. If someone do not notify this error, it is fine,
because the inconsistent patch does not have change to be applied to the
kernel.

> 
> > Also, there's no need to split up the string literal into two lines.
> > It's ok for a line to have more than 80 columns in that case.
> 
> I suggest to run ./scripts/chechpatch.pl before you send any patch.
> It would catch the indentation problems, split of the string, ...

I have used the script checkpatch.pl to verify the patch. And it passed
for the checking. About the indentation problems, it may be caused by
the sepcified vimrc file.

Thanks
Minfei

> 
> Best Regards,
> Petr

  reply	other threads:[~2015-05-18 12:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-12 14:04 [PATCH v3] livepatch: Prevent to apply the patch once coming module notifier fails Minfei Huang
2015-05-13 14:14 ` Josh Poimboeuf
2015-05-14  1:31   ` Minfei Huang
2015-05-18 12:08   ` Petr Mladek
2015-05-18 13:00     ` Minfei Huang [this message]
2015-05-18 15:35       ` Petr Mladek
2015-05-19  4:00         ` Minfei Huang
2015-05-18 15:22     ` Josh Poimboeuf
2015-05-18 15:50       ` Petr Mladek
2015-05-18 15:59         ` Josh Poimboeuf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150518130057.GA25260@dhcp-128-1.nay.redhat.com \
    --to=mhuang@redhat.com \
    --cc=jkosina@suse.cz \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=mnfhuang@gmail.com \
    --cc=pmladek@suse.cz \
    --cc=sjenning@redhat.com \
    --cc=vojtech@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.