Clarification about the series to modernize the tasklet api

Clarification about the series to modernize the tasklet api

[Oscar Carter]

Hi Kees,

I'm working to modernize the tasklet api but I don't understand the reply
to the patch 12/16 [1] of the patch series of Romain Perier [2].

If this patch is combined with the first one, and the function prototypes
are not changed accordingly and these functions don't use the from_tasklet()
helper, all the users that use the DECLARE_TASKLET macro don't pass the
correct argument to the .data field.

 #define DECLARE_TASKLET(name, func, data) \
-struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
+struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), (TASKLET_FUNC_TYPE)func, (TASKLET_DATA_TYPE)&name }

The data argument is lost.

If this patch is splitted in two, the first part will build correctly since
there are casts protecting the arguments, but it will not run correctly until
we apply the second part.

Is it correct? Or am I wrong?

The only imperative to apply a patch in a series is that it compiles correctly?
And I suppose that the next ones fix this situation.

At this moment I'm very confused. A bit of light about this will help me a lot.
And sorry if this is a trivial question.

[1] https://lore.kernel.org/kernel-hardening/201909301538.CEA6C827@keescook/
[2] https://lore.kernel.org/kernel-hardening/20190929163028.9665-1-romain.perier@gmail.com/

Thanks in advance,
Oscar Carter

Re: Clarification about the series to modernize the tasklet api

[Allen]

Oscar,
>
> I'm working to modernize the tasklet api but I don't understand the reply
> to the patch 12/16 [1] of the patch series of Romain Perier [2].

 Am working on the same too. I did try reaching out to Romain but not luck.
Let's hope we are not duplicating efforts.

> If this patch is combined with the first one, and the function prototypes
> are not changed accordingly and these functions don't use the from_tasklet()
> helper, all the users that use the DECLARE_TASKLET macro don't pass the
> correct argument to the .data field.
>
>  #define DECLARE_TASKLET(name, func, data) \
> -struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
> +struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), (TASKLET_FUNC_TYPE)func, (TASKLET_DATA_TYPE)&name }
>

 Ideally this above bit should have been part of the first patch.

> The data argument is lost.
>
> If this patch is splitted in two, the first part will build correctly since
> there are casts protecting the arguments, but it will not run correctly until
> we apply the second part.
>

I have a few more things to complete, I shall have it done and pushed
to github. Will write back
once that's done.

- Allen

Re: Clarification about the series to modernize the tasklet api

[Allen]

>
> I have a few more things to complete, I shall have it done and pushed
> to github. Will write back
> once that's done.

Here's the link to the list of patches so far. I should hopefully
complete the rest in about
a  week.

https://github.com/allenpais/tasklets/commits/ref_tasklets

What I have done so far is to keep patches close to the original
series, but with changes
from the feedback received to those patches.

Patches 1-10 have been retained as is, except for DECLARE_TASKLET which has been
moved to patch 1 from patch 12 in the series.

Patch 11 is broken down to sub-systems(as it was done for Timer
changes in the past).
Sub-systems:
 arch/*
 drivers/atm/*
 drivers/block/*
 drivers/char/ipmi/*
 drivers/crypto/*
 drivers/dma/*
 drivers/firewire/*
 drivers/gpu/drm/*
 drivers/hsi/*
 drivers/hv/*
 drivers/infiniband/*
 drivers/input/*
 drivers/mailbox/*
 drivers/media/*
 drivers/memstick/*
 drivers/misc/*
 drivers/mmc/*
 drivers/net/*
 drivers/net/ethernet/*
 drivers/net/ppp/*
 drivers/net/usb/*
 drivers/net/wireless/*
 drivers/ntb/*
 drivers/platform/*
 drivers/rapidio/*
 drivers/s390/*
 drivers/scsi/*
 drivers/spi/*
 drivers/tty/*
 drivers/usb/*
 drivers/vme/*
 net/dccp/*
 net/ipv4/*
 net/mac80211/*
 net/mac802154/*
 net/rds/*
 net/sched/*
 net/smc/*
 net/xfrm/*
 sound/core/*
 sound/*
 and ofcourse staging/*

 Patches 12, 13 comments will be addressed and broken down.
 Patches 14, 15 will remain as is.
 Patch 16 will have it's comments addressed.

With this approach, I think it will be a little easier to get the
patches into the kernel and also will be easier for maintainers to have
them reviewed.

Thanks,
-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Kees Cook]

On Mon, Jul 13, 2020 at 03:54:44PM +0530, Allen wrote:
> >
> > I have a few more things to complete, I shall have it done and pushed
> > to github. Will write back
> > once that's done.
> 
> Here's the link to the list of patches so far. I should hopefully
> complete the rest in about
> a  week.
> 
> https://github.com/allenpais/tasklets/commits/ref_tasklets

Thanks for the update! I wonder if there is a way to collaborate better
on this, so we're not all waiting on each other? (i.e. Romain got busy,
Allen picked up the work, then Allen got busy, then Oscar picked up the
work, then Allen returned, etc...)

Perhaps split up testing? I'll like you and Oscar work it out. :)

> What I have done so far is to keep patches close to the original
> series, but with changes
> from the feedback received to those patches.
> 
> Patches 1-10 have been retained as is, except for DECLARE_TASKLET which has been
> moved to patch 1 from patch 12 in the series.

I think the "prepare" patches should be collapsed into the "convert"
patches (i.e. let's just touch a given driver once with a single patch).

> Patch 11 is broken down to sub-systems(as it was done for Timer
> changes in the past).
> Sub-systems:
>  arch/*
>  drivers/atm/*
>  drivers/block/*
>  drivers/char/ipmi/*
>  drivers/crypto/*
>  drivers/dma/*
>  drivers/firewire/*
>  drivers/gpu/drm/*
>  drivers/hsi/*
>  drivers/hv/*
>  drivers/infiniband/*
>  drivers/input/*
>  drivers/mailbox/*
>  drivers/media/*
>  drivers/memstick/*
>  drivers/misc/*
>  drivers/mmc/*
>  drivers/net/*
>  drivers/net/ethernet/*
>  drivers/net/ppp/*
>  drivers/net/usb/*
>  drivers/net/wireless/*
>  drivers/ntb/*
>  drivers/platform/*
>  drivers/rapidio/*
>  drivers/s390/*
>  drivers/scsi/*
>  drivers/spi/*
>  drivers/tty/*
>  drivers/usb/*
>  drivers/vme/*
>  net/dccp/*
>  net/ipv4/*
>  net/mac80211/*
>  net/mac802154/*
>  net/rds/*
>  net/sched/*
>  net/smc/*
>  net/xfrm/*
>  sound/core/*
>  sound/*
>  and ofcourse staging/*
> 
>  Patches 12, 13 comments will be addressed and broken down.
>  Patches 14, 15 will remain as is.
>  Patch 16 will have it's comments addressed.
> 
> With this approach, I think it will be a little easier to get the
> patches into the kernel and also will be easier for maintainers to have
> them reviewed.

Yup -- it's that first patch that needs to land in a release so the rest can start
landing too. (Timing here is the awkward part: the infrastructure change
needs to be in -rc1 or -rc2 so the next development cycle can use it
(i.e. subsystem maintainers effectively fork after the merge window is
over). We can play other tricks (common merged branch) but I don't think
that's needed here.

Thanks to all three of you for poking at this!

-- 
Kees Cook

Re: Clarification about the series to modernize the tasklet api

[Allen]

>
> Thanks for the update! I wonder if there is a way to collaborate better
> on this, so we're not all waiting on each other? (i.e. Romain got busy,
> Allen picked up the work, then Allen got busy, then Oscar picked up the
> work, then Allen returned, etc...)
>
> Perhaps split up testing? I'll like you and Oscar work it out. :)

 Yes, this has been a bit of a problem. But this time, I will ensure to get
this all upstreamed.

> > What I have done so far is to keep patches close to the original
> > series, but with changes
> > from the feedback received to those patches.
> >
> > Patches 1-10 have been retained as is, except for DECLARE_TASKLET which has been
> > moved to patch 1 from patch 12 in the series.
>
> I think the "prepare" patches should be collapsed into the "convert"
> patches (i.e. let's just touch a given driver once with a single patch).

 Yes, that is WIP.

>
> Yup -- it's that first patch that needs to land in a release so the rest can start
> landing too. (Timing here is the awkward part: the infrastructure change
> needs to be in -rc1 or -rc2 so the next development cycle can use it
> (i.e. subsystem maintainers effectively fork after the merge window is
> over). We can play other tricks (common merged branch) but I don't think
> that's needed here.
>

My Plan is to get it all ready by the end of 5.8 so that we are ready
for 5.9 merge.

Thoughts?

-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Oscar Carter]

On Mon, Jul 13, 2020 at 09:27:34AM -0700, Kees Cook wrote:
> On Mon, Jul 13, 2020 at 03:54:44PM +0530, Allen wrote:
> > >
> > > I have a few more things to complete, I shall have it done and pushed
> > > to github. Will write back
> > > once that's done.
> >
> > Here's the link to the list of patches so far. I should hopefully
> > complete the rest in about
> > a  week.
> >
> > https://github.com/allenpais/tasklets/commits/ref_tasklets
>
> Thanks for the update! I wonder if there is a way to collaborate better
> on this, so we're not all waiting on each other? (i.e. Romain got busy,
> Allen picked up the work, then Allen got busy, then Oscar picked up the
> work, then Allen returned, etc...)
>
> Perhaps split up testing? I'll like you and Oscar work it out. :)

Sure. If you need help give me some task. It will be a pleasure to help on
this. The only inconvenience is that my time to work on this is only a few
hours during the weekend. If the task have no timeout I will finish it as
soon as possible. Sorry, I would like to have more time to work on the linux
kernel.

> > What I have done so far is to keep patches close to the original
> > series, but with changes
> > from the feedback received to those patches.
> >
> > Patches 1-10 have been retained as is, except for DECLARE_TASKLET which has been
> > moved to patch 1 from patch 12 in the series.
>
> I think the "prepare" patches should be collapsed into the "convert"
> patches (i.e. let's just touch a given driver once with a single patch).
>
> > Patch 11 is broken down to sub-systems(as it was done for Timer
> > changes in the past).
> > Sub-systems:
> >  arch/*
> >  drivers/atm/*
> >  drivers/block/*
> >  drivers/char/ipmi/*
> >  drivers/crypto/*
> >  drivers/dma/*
> >  drivers/firewire/*
> >  drivers/gpu/drm/*
> >  drivers/hsi/*
> >  drivers/hv/*
> >  drivers/infiniband/*
> >  drivers/input/*
> >  drivers/mailbox/*
> >  drivers/media/*
> >  drivers/memstick/*
> >  drivers/misc/*
> >  drivers/mmc/*
> >  drivers/net/*
> >  drivers/net/ethernet/*
> >  drivers/net/ppp/*
> >  drivers/net/usb/*
> >  drivers/net/wireless/*
> >  drivers/ntb/*
> >  drivers/platform/*
> >  drivers/rapidio/*
> >  drivers/s390/*
> >  drivers/scsi/*
> >  drivers/spi/*
> >  drivers/tty/*
> >  drivers/usb/*
> >  drivers/vme/*
> >  net/dccp/*
> >  net/ipv4/*
> >  net/mac80211/*
> >  net/mac802154/*
> >  net/rds/*
> >  net/sched/*
> >  net/smc/*
> >  net/xfrm/*
> >  sound/core/*
> >  sound/*
> >  and ofcourse staging/*
> >
> >  Patches 12, 13 comments will be addressed and broken down.
> >  Patches 14, 15 will remain as is.
> >  Patch 16 will have it's comments addressed.
> >
> > With this approach, I think it will be a little easier to get the
> > patches into the kernel and also will be easier for maintainers to have
> > them reviewed.
>
> Yup -- it's that first patch that needs to land in a release so the rest can start
> landing too. (Timing here is the awkward part: the infrastructure change
> needs to be in -rc1 or -rc2 so the next development cycle can use it
> (i.e. subsystem maintainers effectively fork after the merge window is
> over). We can play other tricks (common merged branch) but I don't think
> that's needed here.
>
> Thanks to all three of you for poking at this!
>
> --
> Kees Cook

Thanks,
Oscar Carter

Re: Clarification about the series to modernize the tasklet api

[Kees Cook]

On Mon, Jul 13, 2020 at 03:54:44PM +0530, Allen wrote:
> https://github.com/allenpais/tasklets/commits/ref_tasklets

Okay, I took a look at this series. I'd like to introduce the new API
differently than was done for timer_struct because I think it'll be
easier and more CFI-friendly. Here's what I've got:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/tasklets/new-api/v1

All the rest can apply on top of that (though it looks like many patches
need work, I got a lot of errors from "make allmodconfig && make".

How does that look?

-- 
Kees Cook

Re: Clarification about the series to modernize the tasklet api

[Allen]

>
> Okay, I took a look at this series. I'd like to introduce the new API
> differently than was done for timer_struct because I think it'll be
> easier and more CFI-friendly. Here's what I've got:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/tasklets/new-api/v1

 Okay, I took a quick look and I like it. I shall get this in and work
on the rest of
the patches and hopefully get it all ready by the end of this week.

>
> All the rest can apply on top of that (though it looks like many patches
> need work, I got a lot of errors from "make allmodconfig && make".

My bad, I should have told you that github was just a placeholder for
WIP patches. I have been testing a set of patches regularly, no issues.
I shall push the updated branch.

Thank you.
-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Allen]

>
> My bad, I should have told you that github was just a placeholder for
> WIP patches. I have been testing a set of patches regularly, no issues.
> I shall push the updated branch.
>

 Made some progress today. I have a few more things to complete
before the series can go out for review. I have pushed out the changes to:

https://github.com/allenpais/tasklets/commits/tasklets_V1

Thanks,
 - Allen

Re: Clarification about the series to modernize the tasklet api

[Kees Cook]

On Tue, Jul 14, 2020 at 07:57:06PM +0530, Allen wrote:
> >
> > My bad, I should have told you that github was just a placeholder for
> > WIP patches. I have been testing a set of patches regularly, no issues.
> > I shall push the updated branch.
> >
> 
>  Made some progress today. I have a few more things to complete
> before the series can go out for review. I have pushed out the changes to:
> 
> https://github.com/allenpais/tasklets/commits/tasklets_V1

Nice! One style nit is the Subject lines needs spaces after the ":"s.
For example, "crypto:hifn_795x: replace tasklet...." should be
"crypto: hifn_795x: replace tasklet...."

I did this to fix it:

rm 0*.patch
git format-patch 7f51b77d527325bfa1dd5da21810765066bd60ff
git reset --hard 7f51b77d527325bfa1dd5da21810765066bd60ff
perl -pi.old -e \
	's/Subject: \[PATCH([^\]]+)\] ([^:]+):([^ ])/Subject: [PATCH\1] \2: \3/' 0*patch
git am 0*.patch


-- 
Kees Cook

Re: Clarification about the series to modernize the tasklet api

[Allen]

On Wed, Jul 15, 2020 at 4:50 AM Kees Cook <keescook@chromium.org> wrote:
>
>
> I did this to fix it:
>
> rm 0*.patch
> git format-patch 7f51b77d527325bfa1dd5da21810765066bd60ff
> git reset --hard 7f51b77d527325bfa1dd5da21810765066bd60ff
> perl -pi.old -e \
>         's/Subject: \[PATCH([^\]]+)\] ([^:]+):([^ ])/Subject: [PATCH\1] \2: \3/' 0*patch
> git am 0*.patch

Thanks, I shall have it fixed.

-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Allen]

> >
> > rm 0*.patch
> > git format-patch 7f51b77d527325bfa1dd5da21810765066bd60ff
> > git reset --hard 7f51b77d527325bfa1dd5da21810765066bd60ff
> > perl -pi.old -e \
> >         's/Subject: \[PATCH([^\]]+)\] ([^:]+):([^ ])/Subject: [PATCH\1] \2: \3/' 0*patch
> > git am 0*.patch
>
> Thanks, I shall have it fixed.


Fixed all the patches and cleaned them up.

https://github.com/allenpais/tasklets/commits/tasklets_V2

Build tested with "make allmodconfig" and boot tested on x86,  Raspberry Pi3,
qemu(arm64 and x86).

I will have the drivers in staging converted in a day or two.
I haven't yet added the patch to completely remove the old API, I
think it's best
for the current series to be accepted upstream and only when all of them have
been accepted it will be right to send the clean up patch.

Please take a look and let me know if they are good to go out for review.
-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Kees Cook]

On Wed, Jul 15, 2020 at 08:21:12PM +0530, Allen wrote:
> > >
> > > rm 0*.patch
> > > git format-patch 7f51b77d527325bfa1dd5da21810765066bd60ff
> > > git reset --hard 7f51b77d527325bfa1dd5da21810765066bd60ff
> > > perl -pi.old -e \
> > >         's/Subject: \[PATCH([^\]]+)\] ([^:]+):([^ ])/Subject: [PATCH\1] \2: \3/' 0*patch
> > > git am 0*.patch
> >
> > Thanks, I shall have it fixed.
> 
> 
> Fixed all the patches and cleaned them up.
> 
> https://github.com/allenpais/tasklets/commits/tasklets_V2
> 
> Build tested with "make allmodconfig" and boot tested on x86,  Raspberry Pi3,
> qemu(arm64 and x86).
> 
> I will have the drivers in staging converted in a day or two.
> I haven't yet added the patch to completely remove the old API, I
> think it's best
> for the current series to be accepted upstream and only when all of them have
> been accepted it will be right to send the clean up patch.
> 
> Please take a look and let me know if they are good to go out for review.

Looking good! I would send the first three first, and point to the
conversion tree in the cover letter "this is what's planned" etc (since
the first three need to land before the rest can start going in).

-- 
Kees Cook

Re: Clarification about the series to modernize the tasklet api

[Allen]

>
> Looking good! I would send the first three first, and point to the
> conversion tree in the cover letter "this is what's planned" etc (since
> the first three need to land before the rest can start going in).
>

Sure, will keep an eye out. In case I miss them, please just
drop me a ping and I will send out the series.

Thanks.


-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Kees Cook]

On Wed, Jul 15, 2020 at 08:51:39PM +0530, Allen wrote:
> >
> > Looking good! I would send the first three first, and point to the
> > conversion tree in the cover letter "this is what's planned" etc (since
> > the first three need to land before the rest can start going in).
> >
> 
> Sure, will keep an eye out. In case I miss them, please just
> drop me a ping and I will send out the series.

Oh, maybe we misunderstood each other? Should I send the first three, or
do you want to do it? I'm fine either way.

-- 
Kees Cook

Re: Clarification about the series to modernize the tasklet api

[Allen]

>
> Oh, maybe we misunderstood each other? Should I send the first three, or
> do you want to do it? I'm fine either way.

 I am fine with either, I'll let you decide on it.

-- 
       - Allen

Re: Clarification about the series to modernize the tasklet api

[Kees Cook]

On Mon, Jul 13, 2020 at 02:55:22PM +0530, Allen wrote:
> Oscar,
> >
> > I'm working to modernize the tasklet api but I don't understand the reply
> > to the patch 12/16 [1] of the patch series of Romain Perier [2].
> 
>  Am working on the same too. I did try reaching out to Romain but not luck.
> Let's hope we are not duplicating efforts.
> 
> > If this patch is combined with the first one, and the function prototypes
> > are not changed accordingly and these functions don't use the from_tasklet()
> > helper, all the users that use the DECLARE_TASKLET macro don't pass the
> > correct argument to the .data field.
> >
> >  #define DECLARE_TASKLET(name, func, data) \
> > -struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
> > +struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), (TASKLET_FUNC_TYPE)func, (TASKLET_DATA_TYPE)&name }
> >
> 
>  Ideally this above bit should have been part of the first patch.

Right, the idea was to have a single patch that contained all the
infrastructure changes to support the conversion patches.

-- 
Kees Cook

Re: Clarification about the series to modernize the tasklet api

[Oscar Carter]

On Mon, Jul 13, 2020 at 09:16:12AM -0700, Kees Cook wrote:
> On Mon, Jul 13, 2020 at 02:55:22PM +0530, Allen wrote:
> > Oscar,
> > >
> > > I'm working to modernize the tasklet api but I don't understand the reply
> > > to the patch 12/16 [1] of the patch series of Romain Perier [2].
> >
> >  Am working on the same too. I did try reaching out to Romain but not luck.
> > Let's hope we are not duplicating efforts.

Don't worry. The work done by me for the moment has been to read all the Romain
patches.

> > > If this patch is combined with the first one, and the function prototypes
> > > are not changed accordingly and these functions don't use the from_tasklet()
> > > helper, all the users that use the DECLARE_TASKLET macro don't pass the
> > > correct argument to the .data field.
> > >
> > >  #define DECLARE_TASKLET(name, func, data) \
> > > -struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
> > > +struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), (TASKLET_FUNC_TYPE)func, (TASKLET_DATA_TYPE)&name }
> > >
> >
> >  Ideally this above bit should have been part of the first patch.
>
> Right, the idea was to have a single patch that contained all the
> infrastructure changes to support the conversion patches.

Ok, thanks for the explanation.

> --
> Kees Cook

Thanks,
Oscar Carter