[v2,0/4] livepatch: Add garbage collection for shadow variables

  Hello,

This is the v2 of the livepatch shadow GC patches. The changes are minor since
nobody asked for for big code changes.

Changes from v1:
* Reworked commit messages (Petr)
* Added my SoB which was missing in some patches, or the ordering was wrong. (Josh)
* Change __klp_shadow_get_or_use to __klp_shadow_get_or_add_locked and add a comment (Petr)
* Add lockdep_assert_held on __klp_shadow_get_or_add_locked (Petr)
  about it's meaning (Petr)
* CCing LKML (Josh)

Some observations:
* Petr has reviewed some of the patches that we created. I kept the Reviewed-by
  tags since he wrote the patches some time ago and now he reviewed them again
  on the ML.
* There were questions about possible problems about using klp_shadow_types
  instead of using ids, but Petr already explained that internally it still uses
  the id to find the correct livepatch.
* Regarding the possibility of multiple patches use the same ID, the problem
  already existed before. Petr suggested using a "stringified" version using
  name and id, but nobody has commented yet. I can implement such feature in a
  v3 if necessary.

Marcos Paulo de Souza (2):
  livepatch/shadow: Introduce klp_shadow_type structure
  livepatch/shadow: Add garbage collection of shadow variables

Petr Mladek (2):
  livepatch/shadow: Separate code to get or use pre-allocated shadow
    variable
  livepatch/shadow: Separate code removing all shadow variables for a
    given id

 include/linux/livepatch.h                     |  50 ++-
 kernel/livepatch/core.c                       |  39 +++
 kernel/livepatch/core.h                       |   1 +
 kernel/livepatch/shadow.c                     | 297 +++++++++++++-----
 kernel/livepatch/transition.c                 |   4 +-
 lib/livepatch/test_klp_shadow_vars.c          | 119 ++++---
 samples/livepatch/livepatch-shadow-fix1.c     |  24 +-
 samples/livepatch/livepatch-shadow-fix2.c     |  34 +-
 .../selftests/livepatch/test-shadow-vars.sh   |   2 +-
 9 files changed, 417 insertions(+), 153 deletions(-)
  

On Wed 2022-10-26 16:41:18, Marcos Paulo de Souza wrote:
> Hello,
> 
> This is the v2 of the livepatch shadow GC patches. The changes are minor since
> nobody asked for for big code changes.
> 
> Changes from v1:
> * Reworked commit messages (Petr)
> * Added my SoB which was missing in some patches, or the ordering was wrong. (Josh)
> * Change __klp_shadow_get_or_use to __klp_shadow_get_or_add_locked and add a comment (Petr)
> * Add lockdep_assert_held on __klp_shadow_get_or_add_locked (Petr)
>   about it's meaning (Petr)
> * CCing LKML (Josh)
> 
> Some observations:
> * Petr has reviewed some of the patches that we created. I kept the Reviewed-by
>   tags since he wrote the patches some time ago and now he reviewed them again
>   on the ML.
> * There were questions about possible problems about using klp_shadow_types
>   instead of using ids, but Petr already explained that internally it still uses
>   the id to find the correct livepatch.
> * Regarding the possibility of multiple patches use the same ID, the problem
>   already existed before. Petr suggested using a "stringified" version using
>   name and id, but nobody has commented yet. I can implement such feature in a
>   v3 if necessary.
> 
> Marcos Paulo de Souza (2):
>   livepatch/shadow: Introduce klp_shadow_type structure
>   livepatch/shadow: Add garbage collection of shadow variables
> 
> Petr Mladek (2):
>   livepatch/shadow: Separate code to get or use pre-allocated shadow
>     variable
>   livepatch/shadow: Separate code removing all shadow variables for a
>     given id

From my POV, the patchset is ready for pushing upstream.

Well, we need to get approval from kpatch-build users. Joe described
possible problems in replay for v3, see
https://lore.kernel.org/r/b5fc2891-2fb0-4aa7-01dd-861da22bb7ea@redhat.com

Best Regards,
Petr
  

On Tue, Nov 01, 2022 at 11:43:50AM +0100, Petr Mladek wrote:
> On Wed 2022-10-26 16:41:18, Marcos Paulo de Souza wrote:
> > Hello,
> > 
> > This is the v2 of the livepatch shadow GC patches. The changes are minor since
> > nobody asked for for big code changes.
> > 
> > Changes from v1:
> > * Reworked commit messages (Petr)
> > * Added my SoB which was missing in some patches, or the ordering was wrong. (Josh)
> > * Change __klp_shadow_get_or_use to __klp_shadow_get_or_add_locked and add a comment (Petr)
> > * Add lockdep_assert_held on __klp_shadow_get_or_add_locked (Petr)
> >   about it's meaning (Petr)
> > * CCing LKML (Josh)
> > 
> > Some observations:
> > * Petr has reviewed some of the patches that we created. I kept the Reviewed-by
> >   tags since he wrote the patches some time ago and now he reviewed them again
> >   on the ML.
> > * There were questions about possible problems about using klp_shadow_types
> >   instead of using ids, but Petr already explained that internally it still uses
> >   the id to find the correct livepatch.
> > * Regarding the possibility of multiple patches use the same ID, the problem
> >   already existed before. Petr suggested using a "stringified" version using
> >   name and id, but nobody has commented yet. I can implement such feature in a
> >   v3 if necessary.
> > 
> > Marcos Paulo de Souza (2):
> >   livepatch/shadow: Introduce klp_shadow_type structure
> >   livepatch/shadow: Add garbage collection of shadow variables
> > 
> > Petr Mladek (2):
> >   livepatch/shadow: Separate code to get or use pre-allocated shadow
> >     variable
> >   livepatch/shadow: Separate code removing all shadow variables for a
> >     given id
> 
> From my POV, the patchset is ready for pushing upstream.

Petr, what do you think about merging the first two patches, since they just
cleanups and simplifications?

> 
> Well, we need to get approval from kpatch-build users. Joe described
> possible problems in replay for v3, see
> https://lore.kernel.org/r/b5fc2891-2fb0-4aa7-01dd-861da22bb7ea@redhat.com
> 
> Best Regards,
> Petr
  

On Mon 2023-01-23 14:33:31, Marcos Paulo de Souza wrote:
> On Tue, Nov 01, 2022 at 11:43:50AM +0100, Petr Mladek wrote:
> > On Wed 2022-10-26 16:41:18, Marcos Paulo de Souza wrote:
> > > Hello,
> > > 
> > > This is the v2 of the livepatch shadow GC patches. The changes are minor since
> > > nobody asked for for big code changes.
> > > 
> > > Changes from v1:
> > > * Reworked commit messages (Petr)
> > > * Added my SoB which was missing in some patches, or the ordering was wrong. (Josh)
> > > * Change __klp_shadow_get_or_use to __klp_shadow_get_or_add_locked and add a comment (Petr)
> > > * Add lockdep_assert_held on __klp_shadow_get_or_add_locked (Petr)
> > >   about it's meaning (Petr)
> > > * CCing LKML (Josh)
> > > 
> > > Some observations:
> > > * Petr has reviewed some of the patches that we created. I kept the Reviewed-by
> > >   tags since he wrote the patches some time ago and now he reviewed them again
> > >   on the ML.
> > > * There were questions about possible problems about using klp_shadow_types
> > >   instead of using ids, but Petr already explained that internally it still uses
> > >   the id to find the correct livepatch.
> > > * Regarding the possibility of multiple patches use the same ID, the problem
> > >   already existed before. Petr suggested using a "stringified" version using
> > >   name and id, but nobody has commented yet. I can implement such feature in a
> > >   v3 if necessary.
> > > 
> > > Marcos Paulo de Souza (2):
> > >   livepatch/shadow: Introduce klp_shadow_type structure
> > >   livepatch/shadow: Add garbage collection of shadow variables
> > > 
> > > Petr Mladek (2):
> > >   livepatch/shadow: Separate code to get or use pre-allocated shadow
> > >     variable
> > >   livepatch/shadow: Separate code removing all shadow variables for a
> > >     given id
> > 
> > From my POV, the patchset is ready for pushing upstream.
> 
> Petr, what do you think about merging the first two patches, since they just
> cleanups and simplifications?

Sounds reasonable to me. I am going to push them by the end of the
week if nobody complained in the meantime.

Best Regards,
Petr
  

On Tue 2023-01-24 16:51:08, Petr Mladek wrote:
> On Mon 2023-01-23 14:33:31, Marcos Paulo de Souza wrote:
> > On Tue, Nov 01, 2022 at 11:43:50AM +0100, Petr Mladek wrote:
> > > On Wed 2022-10-26 16:41:18, Marcos Paulo de Souza wrote:
> > > > Hello,
> > > > 
> > > > This is the v2 of the livepatch shadow GC patches. The changes are minor since
> > > > nobody asked for for big code changes.
> > > > 
> > > > Changes from v1:
> > > > * Reworked commit messages (Petr)
> > > > * Added my SoB which was missing in some patches, or the ordering was wrong. (Josh)
> > > > * Change __klp_shadow_get_or_use to __klp_shadow_get_or_add_locked and add a comment (Petr)
> > > > * Add lockdep_assert_held on __klp_shadow_get_or_add_locked (Petr)
> > > >   about it's meaning (Petr)
> > > > * CCing LKML (Josh)
> > > > 
> > > > Some observations:
> > > > * Petr has reviewed some of the patches that we created. I kept the Reviewed-by
> > > >   tags since he wrote the patches some time ago and now he reviewed them again
> > > >   on the ML.
> > > > * There were questions about possible problems about using klp_shadow_types
> > > >   instead of using ids, but Petr already explained that internally it still uses
> > > >   the id to find the correct livepatch.
> > > > * Regarding the possibility of multiple patches use the same ID, the problem
> > > >   already existed before. Petr suggested using a "stringified" version using
> > > >   name and id, but nobody has commented yet. I can implement such feature in a
> > > >   v3 if necessary.
> > > > 
> > > > Marcos Paulo de Souza (2):
> > > >   livepatch/shadow: Introduce klp_shadow_type structure
> > > >   livepatch/shadow: Add garbage collection of shadow variables
> > > > 
> > > > Petr Mladek (2):
> > > >   livepatch/shadow: Separate code to get or use pre-allocated shadow
> > > >     variable
> > > >   livepatch/shadow: Separate code removing all shadow variables for a
> > > >     given id
> > > 
> > > From my POV, the patchset is ready for pushing upstream.
> > 
> > Petr, what do you think about merging the first two patches, since they just
> > cleanups and simplifications?
> 
> Sounds reasonable to me. I am going to push them by the end of the
> week if nobody complained in the meantime.

Josh accepted the idea in the end so we could actually push the entire
patchset. I am not sure if anyone else would like to review it
so I going to wait a bit longer.

Resume:

I am going to push the entire patchset the following week (Wednesday?)
unless anyone asks for more time or finds a problem.

Best Regards,
Petr
  

On 1/26/23 11:35, Petr Mladek wrote:
> 
> Josh accepted the idea in the end so we could actually push the entire
> patchset. I am not sure if anyone else would like to review it
> so I going to wait a bit longer.
> 
> Resume:
> 
> I am going to push the entire patchset the following week (Wednesday?)
> unless anyone asks for more time or finds a problem.
> 

Hi Petr,

Re docs: patches (3) and (4) change the klp_shadow_* API.  There should
be updates (and possibly examples) to
Documentation/livepatch/shadow-vars.rst.

Having this for v1/v2 would have made review a lot easier, though I
understand not wanting to waste cycles on documenting dead ends.
  

On Thu, Jan 26, 2023 at 12:05:02PM -0500, Joe Lawrence wrote:
> On 1/26/23 11:35, Petr Mladek wrote:
> > 
> > Josh accepted the idea in the end so we could actually push the entire
> > patchset. I am not sure if anyone else would like to review it
> > so I going to wait a bit longer.
> > 
> > Resume:
> > 
> > I am going to push the entire patchset the following week (Wednesday?)
> > unless anyone asks for more time or finds a problem.
> > 
> 
> Hi Petr,
> 
> Re docs: patches (3) and (4) change the klp_shadow_* API.  There should
> be updates (and possibly examples) to
> Documentation/livepatch/shadow-vars.rst.

Agreed, a documentation update is definitely needed.

Also, as I mentioned, we need to document the motivation behind the
change and the expected use cases.  Either in the commit log or the
documentation, or even better, some combination.  There was some very
useful background from Nicolai and some very helpful clarifications from
Petr.  We should make sure all that doesn't get lost in depths of the
mailing list.

And, while I did finally accept the idea, I still need to do a more
in-depth review of the patches.  Patches 1-2 look fine, but please give
me some time to properly review patches 3 & 4.
  

On Thu 2023-01-26 10:30:05, Josh Poimboeuf wrote:
> On Thu, Jan 26, 2023 at 12:05:02PM -0500, Joe Lawrence wrote:
> > On 1/26/23 11:35, Petr Mladek wrote:
> > > 
> > > Josh accepted the idea in the end so we could actually push the entire
> > > patchset. I am not sure if anyone else would like to review it
> > > so I going to wait a bit longer.
> > > 
> > > Resume:
> > > 
> > > I am going to push the entire patchset the following week (Wednesday?)
> > > unless anyone asks for more time or finds a problem.
> > > 
> > 
> > Hi Petr,
> > 
> > Re docs: patches (3) and (4) change the klp_shadow_* API.  There should
> > be updates (and possibly examples) to
> > Documentation/livepatch/shadow-vars.rst.
> 
> Agreed, a documentation update is definitely needed.
> 
> Also, as I mentioned, we need to document the motivation behind the
> change and the expected use cases.  Either in the commit log or the
> documentation, or even better, some combination.  There was some very
> useful background from Nicolai and some very helpful clarifications from
> Petr.  We should make sure all that doesn't get lost in depths of the
> mailing list.

Great point. I have completely forgot about it. And did not catch the
request when quickly scaning the older replies yesterday.

> And, while I did finally accept the idea, I still need to do a more
> in-depth review of the patches.  Patches 1-2 look fine, but please give
> me some time to properly review patches 3 & 4.

Sure. It is great that the code will get another review!

I am going to wait for v3 with updated documentation and review.

Best Regards,
Petr
  

On Thu, Jan 26, 2023 at 12:05:02PM -0500, Joe Lawrence wrote:
> On 1/26/23 11:35, Petr Mladek wrote:
> > 
> > Josh accepted the idea in the end so we could actually push the entire
> > patchset. I am not sure if anyone else would like to review it
> > so I going to wait a bit longer.
> > 
> > Resume:
> > 
> > I am going to push the entire patchset the following week (Wednesday?)
> > unless anyone asks for more time or finds a problem.
> > 
> 
> Hi Petr,
> 
> Re docs: patches (3) and (4) change the klp_shadow_* API.  There should
> be updates (and possibly examples) to
> Documentation/livepatch/shadow-vars.rst.

I forgot about shadow-vars.rst! This will be added on v3.

> 
> Having this for v1/v2 would have made review a lot easier, though I
> understand not wanting to waste cycles on documenting dead ends.

That's true. Next time I'll take care of the docs when the API changes. Thanks
for the reviews so far!

> 
> -- 
> Joe
>