Message-ID: <CABVgOSk6cvPHs3CsoG0FgHz9Y1OT31ZCk=eu5cCOXyg03uNpBA@mail.gmail.com>
Date:   Fri, 22 Sep 2023 16:00:21 +0800
From:   David Gow <davidgow@...gle.com>
To:     Arthur Grillo <arthurgrillo@...eup.net>
Cc:     David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Maxime Ripard <mripard@...nel.org>,
        Javier Martinez Canillas <javierm@...hat.com>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        tales.aparecida@...il.com, andrealmeid@...eup.net,
        mairacanal@...eup.net, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, kunit-dev@...glegroups.com
Subject: Re: [PATCH 2/3] kunit: Add kunit_move_action_to_top_or_reset() to
 reorder actions

On Wed, 20 Sept 2023 at 14:12, Arthur Grillo <arthurgrillo@...eup.net> wrote:
>
> On Kunit, if we allocate a resource A and B on this order, with its
> deferred actions to free them. The resource stack would be something
> like this:
>
>          +---------+
>          | free(B) |
>          +---------+
>          |   ...   |
>          +---------+
>          | free(A) |
>          +---------+
>
> If the deferred action of A accesses B, this would cause a
> use-after-free bug. To solve that, we need a way to change the order
> of actions.
>
> Create a function to move an action to the top of the resource stack,
> as shown in the diagram below.
>
>          +---------+    +---------+
>          | free(B) |    | free(A) |
>          +---------+    +---------+
>          |   ...   | -> | free(B) |
>          +---------+    +---------+
>          | free(A) |    |   ...   |
>          +---------+    +---------+
>
> Signed-off-by: Arthur Grillo <arthurgrillo@...eup.net>
> ---

Thanks. This is a really interesting patch: my hope was that something
like this wouldn't be necessary, as in most cases freeing things in
the reverse order to which they were created is the right thing to do.

It looks like, from the comments on patch 3, this may no longer be
necessary? Is that so?

Otherwise, if you have a real use case for it, I've no objection to
KUnit adding this as a feature (though I'd probably add some
documentation suggesting that it's best avoided if you can order your
allocations / calls better).

Cheers,
-- David

>  include/kunit/resource.h | 17 +++++++++++++++++
>  lib/kunit/resource.c     | 19 +++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index c7383e90f5c9..c598b23680e3 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -479,4 +479,21 @@ void kunit_remove_action(struct kunit *test,
>  void kunit_release_action(struct kunit *test,
>                           kunit_action_t *action,
>                           void *ctx);
> +
> +/**
> + * kunit_move_action_to_top_or_reset - Move a previously added action to the top
> + *                                    of the order of actions calls.
> + * @test: Test case to associate the action with.
> + * @action: The function to run on test exit
> + * @ctx: Data passed into @func
> + *
> + * Reorder the action stack, by moving the desired action to the top.
> + *
> + * Returns:
> + *   0 on success, an error if the action could not be inserted on the top.
> + */
> +int kunit_move_action_to_top_or_reset(struct kunit *test,
> +                                     kunit_action_t *action,
> +                                     void *ctx);
> +
>  #endif /* _KUNIT_RESOURCE_H */
> diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
> index f0209252b179..fe40a34b62a6 100644
> --- a/lib/kunit/resource.c
> +++ b/lib/kunit/resource.c
> @@ -176,3 +176,22 @@ void kunit_release_action(struct kunit *test,
>         }
>  }
>  EXPORT_SYMBOL_GPL(kunit_release_action);
> +
> +int kunit_move_action_to_top_or_reset(struct kunit *test,
> +                                     kunit_action_t *action,
> +                                     void *ctx)
> +{
> +       struct kunit_action_ctx match_ctx;
> +       struct kunit_resource *res;
> +
> +       match_ctx.func = action;
> +       match_ctx.ctx = ctx;
> +       res = kunit_find_resource(test, __kunit_action_match, &match_ctx);
> +       if (res) {
> +               kunit_remove_action(test, action, ctx);
> +               return kunit_add_action_or_reset(test, action, ctx);
> +       }
> +

if (!res), this doesn't call the action, so the _or_reset() part of
this doesn't quite make sense.

As I understand it, there are three cases handled here:
1. The action already existed, and we were able to recreate it at the top.
2. The action already existed, but we were unable to recreate it.
3. The action did not previously exist.

In this case, for (1), the action is successfully moved to the top.
This is the "good case".
For (2), we run the action immediately (the idea being that it's
better to not leak memory).
For (3), we do nothing, the action is never run.

My guess, from the name ending in _or_reset, (3) should:
- Try to defer the action. If deferring it fails, run the action immediately.

Or possibly, always run the action immediately in case (3).

Whatever we want, we need to decide on what happens here and document them.

And of course, we can get some of those behaviours without needing to
call kunit_find_resource() at all, just by calling
kunit_remove_action(...)
kunit_add_action_or_reset()
unconditionally.

> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_move_action_to_top_or_reset);
>
> --
> 2.41.0
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists