linux-kernel - Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190129200049.aarlvssu7fihfpwh@treble>
Date:   Tue, 29 Jan 2019 14:00:49 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Joe Lawrence <joe.lawrence@...hat.com>
Cc:     Miroslav Benes <mbenes@...e.cz>, Petr Mladek <pmladek@...e.com>,
        Jiri Kosina <jikos@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in
 struct klp_patch

On Wed, Jan 23, 2019 at 01:27:59PM -0500, Joe Lawrence wrote:
> > I wanted to ask why there is list_empty() and not klp_patch_enabled(), so
> > just to be sure... the patch was added to klp_patches list, so patch->list
> > is not empty (should not be). We could achieve the same by calling
> > !klp_patch_enabled() given its implementation, but it would look
> > counter-intuitive here.
> > 
> > The rest looks fine.
> > 
> > However, I am not sure if the outcome is better than what we have. Yes,
> > patch->enabled is not technically necessary and we can live with that (as
> > the patch proves). On the other hand, it gives the reader clear guidance
> > about the patch's state. klp_patch_enabled() is not a complete
> > replacement. We have to call list_empty() in __klp_enable_patch() or check
> > the original klp_target_state in klp_try_complete_transition().
> > 
> > I am not against the change, I am glad to see it is achievable, but I am
> > not sure if the code is better with it. Joe acked it. What do the others
> > think?
> 
> Let me qualify my ack -- I think minimizing the number of state variables
> like patch->enabled can help readability... on the other hand, deducing the
> same information from other properties like list-empty can be confusing, ie,
> klp_patch_enabled() is generally a lot clearer than
> list_empty(&patch->list)).
> 
> So I like this idea and would be interested to hear what folks think about
> the exception cases you pointed out.

I share Miroslav and Joe's ambivalence.  It's interesting to see that it
can be done, and normally I'd prefer to get rid of extraneous data
fields, but the patch doesn't reduce code, and it even makes the code
slightly more complex IMO, because klp_patch_enabled() doesn't always
work like you'd expect.

So while I suggested it to begin with, I'm going to go with a NACK :-)

-- 
Josh