Re:

[Roman Rashchupkin <raschupkin.ri@gmail.com>]

 >> The first thing that comes to mind is that this might be solved using
 >> the existing shadow variable API.

 > Same.

I just don't have enough experience using live-patch shadow-variables, 
so I agree that probably that's a better general solution for problem 
(1) of refcount underflow, than mine refholder flags.

 > I can confirm that this scenario happens quite often with real world CVE
 > fixes and there's currently no way to implement such changes safely from
 > a livepatch. But I also believe this is an instance of a broader problem
 > class we attempted to solve with that "enhanced" states API proposed and
 > discussed at LPC ([1], there's a link to a recording at the bottom). For
 > reference, see Petr's POC from [2].

About (2) of incorrect refcount_t value left after unpatch, it seems as 
a bit different and more special problem with counters, compared to the 
support of live-patch states, which can be solved for refcount_t in a 
simpler way.

IMHO adding temporary kprefcount_t variable for the time of live-patch 
being applied, modifying only this kprefcount_t variable by all added in 
livepatch inc()/dec(), provides correct refcounting during live-patch is 
applied. Then at the unpatch this temporaray variable can just safely be 
discarded. This way the only additional code to live-patch would be 
functions with original refcount_dec_and_test() calls.

---

Roman Rashchupkin

On 7/16/24 11:28, Nicolai Stange wrote:
> Hi all,
>
> Joe Lawrence <joe.lawrence@redhat.com> writes:
>
>> On Sun, Jul 14, 2024 at 09:59:32PM +0200, raschupkin.ri@gmail.com wrote:
>> But first, let me see if I understand the problem correctly.  Let's say
>> points A and A' below represent the original kernel code reference
>> get/put pairing in task execution flow.  A livepatch adds a new get/put
>> pair, B and B' in the middle like so:
>>
>>    ---  execution flow  --->
>>    -- A  B       B'  A'  -->
>>
>> There are potential issues if the livepatch is (de)activated
>> mid-sequence, between the new pairings:
>>
>>    problem 1:
>>    -- A      .   B'  A'  -->                   'B, but no B =  extra put!
>>              ^ livepatch is activated here
>>
>>    problem 2:
>>    -- A  B   .       A'  -->                   B, but no B' =  extra get!
>>              ^ livepatch is deactivated here
> I can confirm that this scenario happens quite often with real world CVE
> fixes and there's currently no way to implement such changes safely from
> a livepatch. But I also believe this is an instance of a broader problem
> class we attempted to solve with that "enhanced" states API proposed and
> discussed at LPC ([1], there's a link to a recording at the bottom). For
> reference, see Petr's POC from [2].
>
>
>> The first thing that comes to mind is that this might be solved using
>> the existing shadow variable API.
> Same.
>
>
>> When the livepatch takes the new
>> reference (B), it could create a new <struct, NEW_REF> shadow variable
>> instance.  The livepatch code to return the reference (B') would then
>> check on the shadow variable existence before doing so.  This would
>> solve problem 1.
>>
>> The second problem is a little trickier.  Perhaps the shadow variable
>> approach still works as long as a pre-unpatch hook* were to iterate
>> through all the <*, NEW_REF> shadow variable instances and returned
>> their reference before freeing the shadow variable and declaring the
>> livepatch inactive.
> I think the problem of consistently maintaining shadowed reference
> counts (or anything shadowed for that matter) could be solved with the
> help of aforementioned states API enhancements, so I would propose to
> revive Petr's IMO more generic patchset as an alternative.
>
> Thoughts?
>
> Thanks,
>
> Nicolai
>
> [1] https://lpc.events/event/17/contributions/1541/
> [2] https://lore.kernel.org/r/20231110170428.6664-1-pmladek@suse.com
>