refactor tasklets to avoid unsigned long argument

refactor tasklets to avoid unsigned long argument

[Romain Perier]

Hi,

I would be interested by this task (so I will mark it as "WIP" on the
wiki). I just need context :)

Thanks,
Regards,
Romain

Re: refactor tasklets to avoid unsigned long argument

[Kees Cook]

On Tue, Jul 02, 2019 at 09:35:17AM +0200, Romain Perier wrote:
> I would be interested by this task (so I will mark it as "WIP" on the
> wiki). I just need context :)

Sounds good!

This task is similar to the struct timer_list refactoring. Instead of
passing an arbitrary "unsigned long" argument, it's better that the
"parent" structure that holds the tasklet should be found using
container_of(), and the argument should be the tasklet itself.

Let me know if you need more detail on what that should look like! (And
as always, double-check the sanity of this work: perhaps the refactoring
creates more problems than it solves? Part of this work item is
evaluating the work itself.)

Thanks!

-- 
Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

It Looks interesting :)

Mhhh, so If I understand it right, the purpose of this task is to
remove the "unsigned long data"  argument passed to tasklet_init() ,
that
is mostly used to pass the pointer of the parent structure that
contains the tasklet_struct to the handler.

We don't change the API of tasklet, we simply remove the code that use
this "unsigned long data" wrongly to pass the pointer of the parent
structure
(by using container_of() or something equivalent).

For example this is the case in:   drivers/firewire/ohci.c   or
drivers/s390/block/dasd.c  .

Several question come:

1. I am not sure but, do we need to modify the prototype of
tasklet_init() ?  well, this "unsigned long data" might be use for
something else that pass the pointer of the parent struct. So I would
say "no"


2. In term of security, this is a problem ? Or this is just an
improvement to force developpers to do things correctly ?

I will update the WIKI

Thanks,
Regards,
Romain




Le mar. 2 juil. 2019 à 23:04, Kees Cook <keescook@chromium.org> a écrit :
>
> On Tue, Jul 02, 2019 at 09:35:17AM +0200, Romain Perier wrote:
> > I would be interested by this task (so I will mark it as "WIP" on the
> > wiki). I just need context :)
>
> Sounds good!
>
> This task is similar to the struct timer_list refactoring. Instead of
> passing an arbitrary "unsigned long" argument, it's better that the
> "parent" structure that holds the tasklet should be found using
> container_of(), and the argument should be the tasklet itself.
>
> Let me know if you need more detail on what that should look like! (And
> as always, double-check the sanity of this work: perhaps the refactoring
> creates more problems than it solves? Part of this work item is
> evaluating the work itself.)
>
> Thanks!
>
> --
> Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Kees Cook]

On Wed, Jul 03, 2019 at 05:48:42PM +0200, Romain Perier wrote:
> Mhhh, so If I understand it right, the purpose of this task is to
> remove the "unsigned long data"  argument passed to tasklet_init() ,
> that
> is mostly used to pass the pointer of the parent structure that
> contains the tasklet_struct to the handler.

Right. The idea being that when a tasklet is stored in memory, it no
longer contains both the callback function pointer AND the argument to
pass it. This is the same problem that existed for struct timer_list.
You can see more details about this in at the start of the timer_list
refactoring:
https://git.kernel.org/linus/686fef928bba6be13cabe639f154af7d72b63120

> We don't change the API of tasklet, we simply remove the code that use
> this "unsigned long data" wrongly to pass the pointer of the parent
> structure
> (by using container_of() or something equivalent).

Kind of. In the timer_list case, there were some places were actual data
(and not a pointer) was being passed -- those needed some thought to
convert sanely. I'm hoping that the tasklets are a much smaller part of
the kernel and won't pose as much of a problem, but I haven't studied
it.

> For example this is the case in:   drivers/firewire/ohci.c   or
> drivers/s390/block/dasd.c  .

Right:

struct ar_context {
	...
        struct tasklet_struct tasklet;
};

static void ar_context_tasklet(unsigned long data)
{
        struct ar_context *ctx = (struct ar_context *)data;
...

static int ar_context_init(...)
{
	...
        tasklet_init(&ctx->tasklet, ar_context_tasklet, (unsigned long)ctx);


this could instead be:

static void ar_context_tasklet(struct tasklet_struct *tasklet)
{
	struct ar_context *ctx = container_of(tasklet, typeof(*ctx), tasklet);
...

static int ar_context_init(...)
{
	...
        tasklet_setup(&ctx->tasklet, ar_context_tasklet);

> Several question come:
> 
> 1. I am not sure but, do we need to modify the prototype of
> tasklet_init() ?  well, this "unsigned long data" might be use for
> something else that pass the pointer of the parent struct. So I would
> say "no"

Yes, the final step in the refactoring would be to modify the tasklet_init()
prototype. I've included some example commits from the timer_list
refactoring, but look at the history of include/linux/timer.h and
kernel/time/timer.c for more details.

I would expect the refactoring to follow similar changes to timer_list:

- add a new init API (perhaps tasklet_setup() to follow timer_setup()?)
  that passes the tasklet pointer to tasklet_init(), and casts the
  callback.
	https://git.kernel.org/linus/686fef928bba6be13cabe639f154af7d72b63120
- convert all users to the new prototype
	https://git.kernel.org/linus/e99e88a9d2b067465adaa9c111ada99a041bef9a
- remove the "data" member and convert the callback infrastructure to
  pass the tasklet pointer
	https://git.kernel.org/linus/c1eba5bcb6430868427e0b9d1cd1205a07302f06
- and then clean up anything (cast macros, etc)
	https://git.kernel.org/linus/354b46b1a0adda1dd5b7f0bc2a5604cca091be5f

Hopefully tasklet doesn't have a lot of open-coded initialization. This
is what made timer_list such a challenge. Stuff like this:
	https://git.kernel.org/linus/b9eaf18722221ef8b2bd6a67240ebe668622152a

> 2. In term of security, this is a problem ? Or this is just an
> improvement to force developpers to do things correctly ?

It's a reduction in attack surface (attacker has less control
over the argument if the function pointer is overwritten) and it
provides a distinct prototype for CFI, to make is separate from other
functions that take a single unsigned long argument (e.g. before the
timer_list refactoring, all timer callbacks had the same prototype as
native_write_cr4(), making them a powerful target to control on x86).

For examples of the timer_list attacks (which would likely match a
tasklet attack if one got targeted), see "retire_blk_timer" in:
https://googleprojectzero.blogspot.com/2017/05/exploiting-linux-kernel-via-packet.html

There's also some more detail on the timer_list work in my blog post
for v4.15:
https://outflux.net/blog/archives/2018/02/05/security-things-in-linux-v4-15/

> I will update the WIKI

Awesome!

Thanks for looking at this! I hope it's not at bad as timer_list. :)

-- 
Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

Ok, thanks for these explanations.

The task is in progress, you can follow the status here :
https://salsa.debian.org/rperier-guest/linux-tree/tree/tasklet_init
(the commit messages are tagged WIP, I will add a long message and
signed-off-by , when it's done)

Regards,
Romain

Le jeu. 4 juil. 2019 à 00:46, Kees Cook <keescook@chromium.org> a écrit :
>
> On Wed, Jul 03, 2019 at 05:48:42PM +0200, Romain Perier wrote:
> > Mhhh, so If I understand it right, the purpose of this task is to
> > remove the "unsigned long data"  argument passed to tasklet_init() ,
> > that
> > is mostly used to pass the pointer of the parent structure that
> > contains the tasklet_struct to the handler.
>
> Right. The idea being that when a tasklet is stored in memory, it no
> longer contains both the callback function pointer AND the argument to
> pass it. This is the same problem that existed for struct timer_list.
> You can see more details about this in at the start of the timer_list
> refactoring:
> https://git.kernel.org/linus/686fef928bba6be13cabe639f154af7d72b63120
>
> > We don't change the API of tasklet, we simply remove the code that use
> > this "unsigned long data" wrongly to pass the pointer of the parent
> > structure
> > (by using container_of() or something equivalent).
>
> Kind of. In the timer_list case, there were some places were actual data
> (and not a pointer) was being passed -- those needed some thought to
> convert sanely. I'm hoping that the tasklets are a much smaller part of
> the kernel and won't pose as much of a problem, but I haven't studied
> it.
>
> > For example this is the case in:   drivers/firewire/ohci.c   or
> > drivers/s390/block/dasd.c  .
>
> Right:
>
> struct ar_context {
>         ...
>         struct tasklet_struct tasklet;
> };
>
> static void ar_context_tasklet(unsigned long data)
> {
>         struct ar_context *ctx = (struct ar_context *)data;
> ...
>
> static int ar_context_init(...)
> {
>         ...
>         tasklet_init(&ctx->tasklet, ar_context_tasklet, (unsigned long)ctx);
>
>
> this could instead be:
>
> static void ar_context_tasklet(struct tasklet_struct *tasklet)
> {
>         struct ar_context *ctx = container_of(tasklet, typeof(*ctx), tasklet);
> ...
>
> static int ar_context_init(...)
> {
>         ...
>         tasklet_setup(&ctx->tasklet, ar_context_tasklet);
>
> > Several question come:
> >
> > 1. I am not sure but, do we need to modify the prototype of
> > tasklet_init() ?  well, this "unsigned long data" might be use for
> > something else that pass the pointer of the parent struct. So I would
> > say "no"
>
> Yes, the final step in the refactoring would be to modify the tasklet_init()
> prototype. I've included some example commits from the timer_list
> refactoring, but look at the history of include/linux/timer.h and
> kernel/time/timer.c for more details.
>
> I would expect the refactoring to follow similar changes to timer_list:
>
> - add a new init API (perhaps tasklet_setup() to follow timer_setup()?)
>   that passes the tasklet pointer to tasklet_init(), and casts the
>   callback.
>         https://git.kernel.org/linus/686fef928bba6be13cabe639f154af7d72b63120
> - convert all users to the new prototype
>         https://git.kernel.org/linus/e99e88a9d2b067465adaa9c111ada99a041bef9a
> - remove the "data" member and convert the callback infrastructure to
>   pass the tasklet pointer
>         https://git.kernel.org/linus/c1eba5bcb6430868427e0b9d1cd1205a07302f06
> - and then clean up anything (cast macros, etc)
>         https://git.kernel.org/linus/354b46b1a0adda1dd5b7f0bc2a5604cca091be5f
>
> Hopefully tasklet doesn't have a lot of open-coded initialization. This
> is what made timer_list such a challenge. Stuff like this:
>         https://git.kernel.org/linus/b9eaf18722221ef8b2bd6a67240ebe668622152a
>
> > 2. In term of security, this is a problem ? Or this is just an
> > improvement to force developpers to do things correctly ?
>
> It's a reduction in attack surface (attacker has less control
> over the argument if the function pointer is overwritten) and it
> provides a distinct prototype for CFI, to make is separate from other
> functions that take a single unsigned long argument (e.g. before the
> timer_list refactoring, all timer callbacks had the same prototype as
> native_write_cr4(), making them a powerful target to control on x86).
>
> For examples of the timer_list attacks (which would likely match a
> tasklet attack if one got targeted), see "retire_blk_timer" in:
> https://googleprojectzero.blogspot.com/2017/05/exploiting-linux-kernel-via-packet.html
>
> There's also some more detail on the timer_list work in my blog post
> for v4.15:
> https://outflux.net/blog/archives/2018/02/05/security-things-in-linux-v4-15/
>
> > I will update the WIKI
>
> Awesome!
>
> Thanks for looking at this! I hope it's not at bad as timer_list. :)
>
> --
> Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Kees Cook]

On Sun, Jul 21, 2019 at 07:55:33PM +0200, Romain Perier wrote:
> Ok, thanks for these explanations.

(Reminder: please use inline-context email replies instead of
top-posting, this makes threads much easier to read.)

> The task is in progress, you can follow the status here :
> https://salsa.debian.org/rperier-guest/linux-tree/tree/tasklet_init
> (the commit messages are tagged WIP, I will add a long message and
> signed-off-by , when it's done)

Looks good! I wonder if you're able to use Coccinelle to generate the
conversion patch? There appear to be just under 400 callers of
tasklet_init(), which is a lot to type by hand. :)

Also, have you found any other tasklet users that are NOT using
tasklet_init()? The timer_struct conversion had about three ways
to do initialization. :(

-- 
Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

Le lun. 22 juil. 2019 à 19:19, Kees Cook <keescook@chromium.org> a écrit :
>
> On Sun, Jul 21, 2019 at 07:55:33PM +0200, Romain Perier wrote:
> > Ok, thanks for these explanations.
>
> (Reminder: please use inline-context email replies instead of
> top-posting, this makes threads much easier to read.)

Arf, good point. My bad :)

>
>
> Looks good! I wonder if you're able to use Coccinelle to generate the
> conversion patch? There appear to be just under 400 callers of
> tasklet_init(), which is a lot to type by hand. :)

Mmmhhh, I did not thought *at all* to coccinelle for this, good idea.
I am gonna to read some docs about the tool

>
> Also, have you found any other tasklet users that are NOT using
> tasklet_init()? The timer_struct conversion had about three ways
> to do initialization. :(

This is what I was looking before you give me details about the task.
It seems, there
is only one way to init a tasklet. I have just re-checked, it seems ok.

Thanks for your feedbacks,
Regards,
Romain

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

Hi !

Work is in progress (that's an hobby not full time). I am testing the
build with "allyesconfig".
Do you think it is acceptable to change
drivers/mmc/host/renesas_sdhi_internal_dmac.c  to add a pointer to the
"struct device" or to the "host", so
renesas_sdhi_internal_dmac_complete_tasklet_fn() could access "host"
from the tasklet parameter
because currently, it is not possible.
from the tasklet you can access "dma_priv", from "dma_priv" you can
access "priv", then from "priv", you're blocked :)


This is what I have done for now  :
https://salsa.debian.org/rperier-guest/linux-tree/commit/a0e5735129b4118a1df55b02fead6fa9b7996520
   (separately)

Then the handler would be something like:
https://salsa.debian.org/rperier-guest/linux-tree/commit/5fe1eaeb45060a7df10d166cc96e0bdcf0024368
  (scroll down to renesas_sdhi_internal_dmac_complete_tasklet_fn() ).

What do you think ?

Regards,
Romain

Le mar. 23 juil. 2019 à 10:15, Romain Perier <romain.perier@gmail.com> a écrit :
>
> Le lun. 22 juil. 2019 à 19:19, Kees Cook <keescook@chromium.org> a écrit :
> >
> > On Sun, Jul 21, 2019 at 07:55:33PM +0200, Romain Perier wrote:
> > > Ok, thanks for these explanations.
> >
> > (Reminder: please use inline-context email replies instead of
> > top-posting, this makes threads much easier to read.)
>
> Arf, good point. My bad :)
>
> >
> >
> > Looks good! I wonder if you're able to use Coccinelle to generate the
> > conversion patch? There appear to be just under 400 callers of
> > tasklet_init(), which is a lot to type by hand. :)
>
> Mmmhhh, I did not thought *at all* to coccinelle for this, good idea.
> I am gonna to read some docs about the tool
>
> >
> > Also, have you found any other tasklet users that are NOT using
> > tasklet_init()? The timer_struct conversion had about three ways
> > to do initialization. :(
>
> This is what I was looking before you give me details about the task.
> It seems, there
> is only one way to init a tasklet. I have just re-checked, it seems ok.
>
> Thanks for your feedbacks,
> Regards,
> Romain

Re: refactor tasklets to avoid unsigned long argument

[Kees Cook]

On Thu, Aug 08, 2019 at 05:47:29PM +0200, Romain Perier wrote:
> Le mar. 23 juil. 2019 à 10:15, Romain Perier <romain.perier@gmail.com> a écrit :
> >
> > Le lun. 22 juil. 2019 à 19:19, Kees Cook <keescook@chromium.org> a écrit :
> > >
> > > On Sun, Jul 21, 2019 at 07:55:33PM +0200, Romain Perier wrote:
> > > > Ok, thanks for these explanations.
> > >
> > > (Reminder: please use inline-context email replies instead of
> > > top-posting, this makes threads much easier to read.)
> >
> > Arf, good point. My bad :)
> >
> > >
> > >
> > > Looks good! I wonder if you're able to use Coccinelle to generate the
> > > conversion patch? There appear to be just under 400 callers of
> > > tasklet_init(), which is a lot to type by hand. :)
> >
> > Mmmhhh, I did not thought *at all* to coccinelle for this, good idea.
> > I am gonna to read some docs about the tool
> >
> > >
> > > Also, have you found any other tasklet users that are NOT using
> > > tasklet_init()? The timer_struct conversion had about three ways
> > > to do initialization. :(
> >
> > This is what I was looking before you give me details about the task.
> > It seems, there
> > is only one way to init a tasklet. I have just re-checked, it seems ok.
> 
> Work is in progress (that's an hobby not full time). I am testing the
> build with "allyesconfig".

That's good -- I tend to use allmodconfig (since it sort of tests a
larger set of functions -- the module init code is more complex than the
static init code, IIRC), but I think for this series, you're fine either
way.

> Do you think it is acceptable to change
> drivers/mmc/host/renesas_sdhi_internal_dmac.c  to add a pointer to the
> "struct device" or to the "host", so
> renesas_sdhi_internal_dmac_complete_tasklet_fn() could access "host"
> from the tasklet parameter
> because currently, it is not possible.
> from the tasklet you can access "dma_priv", from "dma_priv" you can
> access "priv", then from "priv", you're blocked :)
> 
> 
> This is what I have done for now  :
> https://salsa.debian.org/rperier-guest/linux-tree/commit/a0e5735129b4118a1df55b02fead6fa9b7996520
>    (separately)
> 
> Then the handler would be something like:
> https://salsa.debian.org/rperier-guest/linux-tree/commit/5fe1eaeb45060a7df10d166cc96e0bdcf0024368
>   (scroll down to renesas_sdhi_internal_dmac_complete_tasklet_fn() ).

I did things like this in a few cases for timer_struct, yes. The only
question I have is if "struct device" is what you want or "struct
platform_device" is what you want?

+	priv->dev = &pdev->dev;

You're already dereferencing "pdev" to get "dev", and then:

+	struct platform_device *pdev = container_of(priv->dev, typeof(*pdev), dev);

What you really want is the pdev anyway in the handler. Maybe just store
that instead?

Also, I think you can avoid the "dma_priv" variable with a from_tasklet()
that uses dma_priv.dma_complete. Something like:

struct renesas_sdhi *priv = from_tasklet(priv, t, dma_priv.dma_complete);

The only other gotcha to check is if it's ever possible for the pointer
you're storing to change through some other means, which would cause you
to be doing a use-after-free in this handler? (I assume not, since dma
completion is tied to the device...)

-- 
Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]

On Thu, Aug 08, 2019 at 02:02:52PM -0700, Kees Cook wrote:
> On Thu, Aug 08, 2019 at 05:47:29PM +0200, Romain Perier wrote:
> > Le mar. 23 juil. 2019 à 10:15, Romain Perier <romain.perier@gmail.com> a écrit :
> > >
> > > Le lun. 22 juil. 2019 à 19:19, Kees Cook <keescook@chromium.org> a écrit :
> > > >
> > > > On Sun, Jul 21, 2019 at 07:55:33PM +0200, Romain Perier wrote:
> > > > > Ok, thanks for these explanations.
> > > >
> > > > (Reminder: please use inline-context email replies instead of
> > > > top-posting, this makes threads much easier to read.)
> > >
> > > Arf, good point. My bad :)
> > >
> > > >
> > > >
> > > > Looks good! I wonder if you're able to use Coccinelle to generate the
> > > > conversion patch? There appear to be just under 400 callers of
> > > > tasklet_init(), which is a lot to type by hand. :)
> > >
> > > Mmmhhh, I did not thought *at all* to coccinelle for this, good idea.
> > > I am gonna to read some docs about the tool
> > >
> > > >
> > > > Also, have you found any other tasklet users that are NOT using
> > > > tasklet_init()? The timer_struct conversion had about three ways
> > > > to do initialization. :(
> > >
> > > This is what I was looking before you give me details about the task.
> > > It seems, there
> > > is only one way to init a tasklet. I have just re-checked, it seems ok.
> > 
> > Work is in progress (that's an hobby not full time). I am testing the
> > build with "allyesconfig".
> 
> That's good -- I tend to use allmodconfig (since it sort of tests a
> larger set of functions -- the module init code is more complex than the
> static init code, IIRC), but I think for this series, you're fine either
> way.
> 

Oh, good to know (I did not know allmodconfig). Yeah I think that it is enough
for this series, but that's a good idea for the other ones :)



> > Do you think it is acceptable to change
> > drivers/mmc/host/renesas_sdhi_internal_dmac.c  to add a pointer to the
> > "struct device" or to the "host", so
> > renesas_sdhi_internal_dmac_complete_tasklet_fn() could access "host"
> > from the tasklet parameter
> > because currently, it is not possible.
> > from the tasklet you can access "dma_priv", from "dma_priv" you can
> > access "priv", then from "priv", you're blocked :)
> > 
> > 
> > This is what I have done for now  :
> > https://salsa.debian.org/rperier-guest/linux-tree/commit/a0e5735129b4118a1df55b02fead6fa9b7996520
> >    (separately)
> > 
> > Then the handler would be something like:
> > https://salsa.debian.org/rperier-guest/linux-tree/commit/5fe1eaeb45060a7df10d166cc96e0bdcf0024368
> >   (scroll down to renesas_sdhi_internal_dmac_complete_tasklet_fn() ).
> 
> I did things like this in a few cases for timer_struct, yes. The only
> question I have is if "struct device" is what you want or "struct
> platform_device" is what you want?
> 
> +	priv->dev = &pdev->dev;
> 
> You're already dereferencing "pdev" to get "dev", and then:
> 
> +	struct platform_device *pdev = container_of(priv->dev, typeof(*pdev), dev);
> 
> What you really want is the pdev anyway in the handler. Maybe just store
> that instead?

Yup, this is what I have done after sending the previous email ;)

> 
> Also, I think you can avoid the "dma_priv" variable with a from_tasklet()
> that uses dma_priv.dma_complete. Something like:
> 
> struct renesas_sdhi *priv = from_tasklet(priv, t, dma_priv.dma_complete);
> 

Mhhh, I thought that container_of() was only working for "1-level" (so
just take the pointer of the parent structure), indeed when you take
a look at how the macro is defined, it make sense. It will make the
code easier to read. Interesting... !

> The only other gotcha to check is if it's ever possible for the pointer
> you're storing to change through some other means, which would cause you
> to be doing a use-after-free in this handler? (I assume not, since dma
> completion is tied to the device...)
> 

I think not in this case, but I agree, that's also preferable for this
reason.

Thanks for your feedbacks,
Regards,
Romain

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Mon, Aug 12, 2019 at 07:29:51PM +0200, Romain Perier wrote:

Hi !

https://salsa.debian.org/rperier-guest/linux-tree/tree/tasklet_init

It is mostly done ! I have just finished the commit for removing the data field
... and... I have completly forgot the macro DECLARE_TASKLET() :=D . Well, it
is not a big issue because there are only few calls.

What I can do is the following:

1. After the commit that convert all tasklet_init() to tasklet_setup(),
I can a new commit that modifies the content of DECLARE_TASKLET()
(pass the pointer of the callback as .data) and convert the callback of all
DECLARE_TASKLET() for handling the argument with from_tasklet() correctly

2. Then the commit for removing the .data field in the tasklet_struct
structure that also removes the data field in DECLARE_TASKLET() (without
changing the API of the macro, I just remove the field data from the
content of the macro)

3. Change the API of DECLARE_TASKLET() and update all calls in a single
shot


What do you think ?

Thanks,
Regards,
Romain

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: refactor tasklets to avoid unsigned long argument

[Kees Cook]

On Thu, Aug 29, 2019 at 08:13:21PM +0200, Romain Perier wrote:
> On Mon, Aug 12, 2019 at 07:29:51PM +0200, Romain Perier wrote:
> 
> Hi !
> 
> https://salsa.debian.org/rperier-guest/linux-tree/tree/tasklet_init
> 
> It is mostly done ! I have just finished the commit for removing the data field
> ... and... I have completly forgot the macro DECLARE_TASKLET() :=D . Well, it
> is not a big issue because there are only few calls.

Heh, oops. Yeah, I kept tripping over things like that with the
timer_struct too.

> What I can do is the following:
> 
> 1. After the commit that convert all tasklet_init() to tasklet_setup(),
> I can a new commit that modifies the content of DECLARE_TASKLET()
> (pass the pointer of the callback as .data) and convert the callback of all
> DECLARE_TASKLET() for handling the argument with from_tasklet() correctly
> 
> 2. Then the commit for removing the .data field in the tasklet_struct
> structure that also removes the data field in DECLARE_TASKLET() (without
> changing the API of the macro, I just remove the field data from the
> content of the macro)

Yup, I think that's the best approach. The .data removal is basically
the last step (well, and the removal of tasklet_init() and the
TASKLET_*_TYPE macros).

Also, looking at your tree: I don't think you need to add the cocci
script to the tree (since you'll just be removing it). I just included
the script in my commit log for the bulk refactoring commit.

In the "tasklet: Prepare to change tasklet callback argument type"
commit, perhaps reference the timer_struct conversion series too, if
people want to see the earlier conversion methods.

English nit pick: "Prepare to the new tasklet API" I would phrase as
"Prepare to use the new tasklet API" or "Prepare for new tasklet API".

-- 
Kees Cook

Re: refactor tasklets to avoid unsigned long argument

[Romain Perier]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

Hi !

I have realized that I did not answer to your previous email (I was sure
that I did it... my bad). Thanks for all your advices.

Well, I have just sent the whole series for a "pre-review" on the ML,
it will avoid common errors, once we agree on the series, I will resend
it to the right people directly.

Sorry for the delay, I was busy these last days :)

Regards,
Romain

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]