>> 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@xxxxxxxxxx> writes:
On Sun, Jul 14, 2024 at 09:59:32PM +0200, raschupkin.ri@xxxxxxxxx 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@xxxxxxxx
