[prev in list] [next in list] [prev in thread] [next in thread] 

List:       live-patching
Subject:    Re: [PATCH v9] livepatch: Clear relocation targets on a module removal
From:       Josh Poimboeuf <jpoimboe () kernel ! org>
Date:       2023-01-24 22:32:41
Message-ID: 20230124223241.ywbhxdrj26qfgtd6 () treble
[Download RAW message or body]

On Tue, Jan 24, 2023 at 03:08:27PM -0500, Joe Lawrence wrote:
> > +	/*
> > +	 * For a livepatch relocation, the restore r2 instruction might have
> > +	 * been previously written if the relocation references a symbol in a
> > +	 * module which was unloaded and is now being reloaded.  In that case,
> > +	 * skip the warning and instruction write.
> > +	 */
> > +	if (klp_sym && insn_val == PPC_INST_LD_TOC)
> > +		return 0;
> 
> Hi Josh,
> 
> Nit: shouldn't this return 1?
> 
> And if you're willing to entertain a small refactor, wouldn't
> restore_r2() be clearer if it returned -ESOMETHING on error?
> 
> Maybe converting to a boolean could work, but then I'd suggest a name
> that clearly implies success/fail given true/false return.  Maybe
> replace_nop_with_ld_toc() or replace_nop_to_restore_r2() ... still
> -ESOMETHING is more intuitive to me as there are cases like this where
> the function safely returns w/o replacing anything.

Indeed, and I actually already discovered that and made such changes,
just need to get around to posting the patches.

-- 
Josh
[prev in list] [next in list] [prev in thread] [next in thread]