Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1723733 > unrolled thread

Converting struct timer_list callback argument to struct timer_list *

Back to article view | Back to linux.kernel

Contents

  Converting struct timer_list callback argument to struct timer_list * Kees Cook <keescook@chromium.org> - 2017-08-31 01:40 +0200
    Re: Converting struct timer_list callback argument to struct timer_list  * Thomas Gleixner <tglx@linutronix.de> - 2017-08-31 22:20 +0200
      Re: Converting struct timer_list callback argument to struct  timer_list * Kees Cook <keescook@chromium.org> - 2017-09-01 01:40 +0200
    Re: Converting struct timer_list callback argument to struct  timer_list * Christoph Hellwig <hch@infradead.org> - 2017-09-01 12:10 +0200
      Re: Converting struct timer_list callback argument to struct timer_list  * Thomas Gleixner <tglx@linutronix.de> - 2017-09-01 12:30 +0200
        Re: Converting struct timer_list callback argument to struct  timer_list * Christoph Hellwig <hch@infradead.org> - 2017-09-01 12:50 +0200
          Re: Converting struct timer_list callback argument to struct  timer_list * Kees Cook <keescook@chromium.org> - 2017-09-01 18:30 +0200
            Re: Converting struct timer_list callback argument to struct  timer_list * Christoph Hellwig <hch@infradead.org> - 2017-09-03 09:40 +0200
              Re: Converting struct timer_list callback argument to struct  timer_list * Kees Cook <keescook@chromium.org> - 2017-09-03 22:20 +0200
                Re: Converting struct timer_list callback argument to struct  timer_list * Christoph Hellwig <hch@infradead.org> - 2017-09-04 08:10 +0200

#1723733 — Converting struct timer_list callback argument to struct timer_list *

Hi,

I'm working on converting all callbacks of struct timer_list into
taking the struct timer_list * that triggered the callback (instead of
the timer_list.data field), as done for hrtimers, work queues, etc.
Doing this has a number of benefits including actual type checking
(via container_of(), etc) instead of blind type casts, and getting rid
of the .data field which is used in security flaw exploits[1].

I've got the series well on its way, though I think I have something
like 400 users remaining, and they're getting less trivial to convert.
:)

The series goes through several phases of conversion. The series is
here to browse:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/timer/20170830

Replace open-coded variants of setup_timer() with actual setup_timer() calls:
      coccinelle: Improve setup_timer.cocci matching
      Run setup_timer.cocci
      timer: Remove init_timer_pinned_deferrable() in favor of
setup_pinned_deferrable_timer()
      timer: Remove init_timer_on_stack() in favor of setup_timer_on_stack()
      timer: Remove init_timer_pinned() in favor of setup_pinned_timer()
      timer: Remove init_timer_deferrable() in favor of setup_deferrable_timer()

Remove little or unused macros, with the goal of removing to ability
to statically define the .data field:
      timer: Remove users of TIMER_DEFERRED_INITIALIZER
      timer: Remove users of TIMER_INITIALIZER
      timer: Remove unused static initializer macros
      timer: Remove users of expire and data arguments to DEFINE_TIMER
      timer: Remove expires and data arguments from DEFINE_TIMER
      timer: Remove expires argument from __TIMER_INITIALIZER()

Fix usages of setup_timer() that Coccinelle can't reason about:
      timer: Remove meaningless .data/.function assignments
      timer: Collapse cross-function single-assignment .data into setup_timer()
      timer: Additional init_timer() -> setup_timer() conversions

Eliminate all open-coded usage of the .data field:
      usb/phy-isp1301-omap: Remove .data assignment
      media/i2c/tc358743: Initialize timer
      scsi/aic7xxx: Clean up timer usage
      timer: Fix incorrect casts in callbacks
      net/core: Collapse redundant sk_timer callback data assignments
      s390/char/sclp: Use separate static data field with with static timer
      sparc/led: Use separate static data field with with static timer
      mips/sgi-ip32: Use separate static data field with with static timer
      mips/sgi-ip22: Use separate static data field with with static timer
      net/atm/mpc: Use separate static data field with with static timer
      staging/comedi/das16: Make timer initialization unconditional
      usb/gadget/snps_udc_core: Move timer initialization earlier
      infiniband/rdmavt: Remove redundant timer initialization
      scsi/bnx2i: Initialize timer
      appletalk: Remove unneeded synchronization
      timer: Switch to testing for .function instead of .data

Create temporary casts for function and data arguments so
setup_timer() will take both old and new style function and data
arguments:
      timer: Temporarily explicitly cast all callback functions

Convert all DEFINE_TIMER users to taking struct timer_list *:
      coccinelle: Add define_timer.cocci
      timer: Switch DEFINE_TIMER callbacks to struct timer_list *

Create TIMER_CONTAINER() help for new style callbacks:
      timer: Add helper for container_of() usage on callbacks

Convert timer users that Coccinelle can't reason about:
      scsi/ipr: Convert timer callbacks to use TIMER_CONATINER
      staging/wilc1000: Convert timer callbacks to use TIMER_CONTAINER
      amifloppy: Convert timer callbacks to use TIMER_CONTAINER
      net/irda: Convert timer callbacks to use TIMER_CONTAINER
      led/ledtrig-heartbeat: Convert callback to use TIMER_CONTAINER
      kthread: Convert callback to use TIMER_CONTAINER
      workqueue: Convert callback to use TIMER_CONTAINER
      scsi/pmcraid: Convert callbacks to use TIMER_CONTAINER
      net/decnet: Convert callback to use TIMER_CONTAINER
      net/lapb: Convert callbacks to use TIMER_CONTAINER
      net/mac80211/mesh_plink: Convert callback to use TIMER_CONTAINER
      fs/nilfs2: Convert callback to use TIMER_CONTAINER
      net/rose: Convert callbacks to use TIMER_CONTAINER
      scsi/sas: Convert callback to use TIMER_CONTAINER
      media/saa7146: Convert callback to use TIMER_CONTAINER
      net/irda-usb: Convert callback to use TIMER_CONTAINER
      input: Convert callbacks to use TIMER_CONTAINER
      net/irda/bfin_sir: Convert callback to use TIMER_CONTAINER
      ALSA: sh: aica: Convert callback to use TIMER_CONTAINER
      s390/tape: Convert callback to use TIMER_CONTAINER
      net/ti/tlan: Convert callback to use TIMER_CONTAINER
      net/wireless/ray_cs: Convert callback to use TIMER_CONTAINER
      firewire: Convert callback to use TIMER_CONTAINER
      sound/pci/asihpi: Convert callback to use TIMER_CONTAINER
      isdn/hisax: Convert callback to use TIMER_CONTAINER
      net/hamradio/6pack: Convert callback to use TIMER_CONTAINER
      pci/ehp_hpc: Convert callback to use TIMER_CONTAINER
      net/usb/usbnet: Convert callback to use TIMER_CONTAINER
      tty/sysrq: Convert callback to use TIMER_CONTAINER
      scsi/cxgbi: Convert callback to use TIMER_CONTAINER
      block/laptop_mode: Convert callback to use TIMER_CONTAINER
      libata: Convert callback to use TIMER_CONTAINER
      staging: rtl8723bs: Refactor timers to use setup_timer()
      staging: rtl8723bs: Convert callback to use TIMER_CONTAINER
      staging: rtl8712: Convert callbacks to use TIMER_CONTAINER
      staging: rtl8188eu: Convert callbacks to use TIMER_CONTAINER
      sound/core: Convert callbacks to use TIMER_CONTAINER
      fs/ncpfs: Convert callback to use TIMER_CONTAINER
      block/aoe: Convert callbacks to use TIMER_CONTAINER

Rename .data field to .cb_data just to find any remaining .data users
that allmodconfig misses but 0-day can find (the above conversions
could be rearranged to do all .data-using conversions first, then this
patch, then the rest of the conversions):
      timer: Rename timer_list.data to timer_list.cb_data

Add Coccinelle script to perform "easy" conversions of timer callbacks:
      coccinelle: Add new timer_list handling scripts
      timer: Mass-convert timer_list callbacks to use TIMER_CONTAINER
      timer: setup_deferrable_timer conversions
      timer: setup_pinned_timer conversions

Which gets me to here:
 747 files changed, 3363 insertions(+), 3410 deletions(-)

And current builds with the forced casts removed show about 400 more
call sites to fix...

Does this look alright, and should some of this go into -next right
now, just to get some of the clean-ups into the tree? (Basically,
everything before "timer: Temporarily explicitly cast all callback
functions")

Thanks!

-Kees

[1] https://lwn.net/Articles/731082/

-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1724554 — Re: Converting struct timer_list callback argument to struct timer_list *

On Wed, 30 Aug 2017, Kees Cook wrote:
> Which gets me to here:
>  747 files changed, 3363 insertions(+), 3410 deletions(-)
> 
> And current builds with the forced casts removed show about 400 more
> call sites to fix...

That's a herculean task. Chapeau!

> Does this look alright,

Very well done. I like the temporary typecast trick.

> and should some of this go into -next right
> now, just to get some of the clean-ups into the tree? (Basically,
> everything before "timer: Temporarily explicitly cast all callback
> functions")

Yes, just get that part in now.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1724653 — Re: Converting struct timer_list callback argument to struct timer_list *

On Thu, Aug 31, 2017 at 1:13 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 30 Aug 2017, Kees Cook wrote:
>> Which gets me to here:
>>  747 files changed, 3363 insertions(+), 3410 deletions(-)
>>
>> And current builds with the forced casts removed show about 400 more
>> call sites to fix...
>
> That's a herculean task. Chapeau!

Coccinelle has helped a lot, but yeah, not a small conversion! :P

>> Does this look alright,
>
> Very well done. I like the temporary typecast trick.

Okay, good. Since there's no sensible type checking here already, it
didn't seem _worse_. :)

>> and should some of this go into -next right
>> now, just to get some of the clean-ups into the tree? (Basically,
>> everything before "timer: Temporarily explicitly cast all callback
>> functions")
>
> Yes, just get that part in now.

Okay, I've sent the first 31 patches your way just now. I'll keep
grinding away at the rest...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1724934 — Re: Converting struct timer_list callback argument to struct timer_list *

Good work!

I just think that the TIMER_CONTAINER name is revolting.

The usual name for such a helper fitting other uses like lists
and rbtrees would be timer_entry, and that also reads much better.

[toc] | [prev] | [next] | [standalone]

#1724942 — Re: Converting struct timer_list callback argument to struct timer_list *

On Fri, 1 Sep 2017, Christoph Hellwig wrote:

> Good work!
> 
> I just think that the TIMER_CONTAINER name is revolting.
> 
> The usual name for such a helper fitting other uses like lists
> and rbtrees would be timer_entry, and that also reads much better.

I think the plan is to remove that thing afterward, because then the
callback function is:

	 void func(struct timer_list *timer)

So I don't mind the ugly name as it should be simply removed once the tree
is converted over.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1724962 — Re: Converting struct timer_list callback argument to struct timer_list *

On Fri, Sep 01, 2017 at 12:21:46PM +0200, Thomas Gleixner wrote:
> On Fri, 1 Sep 2017, Christoph Hellwig wrote:
> 
> > Good work!
> > 
> > I just think that the TIMER_CONTAINER name is revolting.
> > 
> > The usual name for such a helper fitting other uses like lists
> > and rbtrees would be timer_entry, and that also reads much better.
> 
> I think the plan is to remove that thing afterward, because then the
> callback function is:
> 
> 	 void func(struct timer_list *timer)
> 
> So I don't mind the ugly name as it should be simply removed once the tree
> is converted over.

Well, we can't just remove it, we could just replace it with
container_of().  lists and rbtrees just keep their list_entry and
rb_entry wrappers for timer_of, so we could save us the additional
churn by naming it timer_entry and just keeping it.

[toc] | [prev] | [next] | [standalone]

#1725198 — Re: Converting struct timer_list callback argument to struct timer_list *

On Fri, Sep 1, 2017 at 3:44 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Sep 01, 2017 at 12:21:46PM +0200, Thomas Gleixner wrote:
>> On Fri, 1 Sep 2017, Christoph Hellwig wrote:
>>
>> > Good work!
>> >
>> > I just think that the TIMER_CONTAINER name is revolting.
>> >
>> > The usual name for such a helper fitting other uses like lists
>> > and rbtrees would be timer_entry, and that also reads much better.
>>
>> I think the plan is to remove that thing afterward, because then the
>> callback function is:
>>
>>        void func(struct timer_list *timer)
>>
>> So I don't mind the ugly name as it should be simply removed once the tree
>> is converted over.

The removed casts would be the (TIMER_FUNC_TYPE) and (TIMER_DATA_TYPE)
casts. TIMER_CONTAINER is ugly, yes. I can rework that.

> Well, we can't just remove it, we could just replace it with
> container_of().  lists and rbtrees just keep their list_entry and
> rb_entry wrappers for timer_of, so we could save us the additional
> churn by naming it timer_entry and just keeping it.

These examples are:

#define list_entry(ptr, type, member) container_of(ptr, type, member)
#define rb_entry(ptr, type, member) container_of(ptr, type, member)

The use of a "timer_entry()" at the start of callbacks repeats the
struct name, which I find irritating (and it usually results in split
lines). For example:

#define timer_entry(ptr, type, member) container_of(ptr, type, member)

-static void snd_card_asihpi_timer_function(unsigned long data)
+static void snd_card_asihpi_timer_function(struct timer_list *t)
 {
-       struct snd_card_asihpi_pcm *dpcm = (struct snd_card_asihpi_pcm *)data;
+       struct snd_card_asihpi_pcm *dpcm =
+                                timer_entry(t, struct
snd_card_asihpi_pcm, timer);

I prefer to tie this directly to the variable, so how about renaming
TIMER_CONTAINER to timer_of():

#define timer_of(var, callback_timer, timer_fieldname) \
        container_of(callback_timer, typeof(*var), timer_fieldname)

-static void snd_card_asihpi_timer_function(unsigned long data)
+static void snd_card_asihpi_timer_function(struct timer_list *t)
 {
-       struct snd_card_asihpi_pcm *dpcm = (struct snd_card_asihpi_pcm *)data;
+       struct snd_card_asihpi_pcm *dpcm = timer_of(dpcm, t, timer);

If not, I can just go the timer_entry() way, it just means I have to
split a lot of lines.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1725620 — Re: Converting struct timer_list callback argument to struct timer_list *

On Fri, Sep 01, 2017 at 09:24:22AM -0700, Kees Cook wrote:
> #define list_entry(ptr, type, member) container_of(ptr, type, member)
> #define rb_entry(ptr, type, member) container_of(ptr, type, member)
> 
> The use of a "timer_entry()" at the start of callbacks repeats the
> struct name, which I find irritating (and it usually results in split
> lines). For example:
> 
> #define timer_entry(ptr, type, member) container_of(ptr, type, member)
> 
> -static void snd_card_asihpi_timer_function(unsigned long data)
> +static void snd_card_asihpi_timer_function(struct timer_list *t)
>  {
> -       struct snd_card_asihpi_pcm *dpcm = (struct snd_card_asihpi_pcm *)data;
> +       struct snd_card_asihpi_pcm *dpcm =
> +                                timer_entry(t, struct
> snd_card_asihpi_pcm, timer);
> 
> I prefer to tie this directly to the variable, so how about renaming
> TIMER_CONTAINER to timer_of():

The TIMER_CONTAINER semantics are more useful indeed, and I which
we'd have a general purpose variant of that.  But I was complaining
about the name anyway.  timer_of sounds ok, but timer_entry still sounds
a bit more descriptive.  As for the split lines:  you'll generally
get a lot of these, even TIMER_CONTAINER has a quite a few.  I generally
prefer to move everything right of the = to the next line as that
becomes a lot more redable.

[toc] | [prev] | [next] | [standalone]

#1725748 — Re: Converting struct timer_list callback argument to struct timer_list *

On Sun, Sep 3, 2017 at 12:37 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Sep 01, 2017 at 09:24:22AM -0700, Kees Cook wrote:
>> #define list_entry(ptr, type, member) container_of(ptr, type, member)
>> #define rb_entry(ptr, type, member) container_of(ptr, type, member)
>>
>> The use of a "timer_entry()" at the start of callbacks repeats the
>> struct name, which I find irritating (and it usually results in split
>> lines). For example:
>>
>> #define timer_entry(ptr, type, member) container_of(ptr, type, member)
>>
>> -static void snd_card_asihpi_timer_function(unsigned long data)
>> +static void snd_card_asihpi_timer_function(struct timer_list *t)
>>  {
>> -       struct snd_card_asihpi_pcm *dpcm = (struct snd_card_asihpi_pcm *)data;
>> +       struct snd_card_asihpi_pcm *dpcm =
>> +                                timer_entry(t, struct
>> snd_card_asihpi_pcm, timer);
>>
>> I prefer to tie this directly to the variable, so how about renaming
>> TIMER_CONTAINER to timer_of():
>
> The TIMER_CONTAINER semantics are more useful indeed, and I which
> we'd have a general purpose variant of that.  But I was complaining
> about the name anyway.  timer_of sounds ok, but timer_entry still sounds
> a bit more descriptive.  As for the split lines:  you'll generally
> get a lot of these, even TIMER_CONTAINER has a quite a few.  I generally
> prefer to move everything right of the = to the next line as that
> becomes a lot more redable.

It feels weird to have different semantics from container_of() too, so
I'll probably just switch everything around to be like all the others,
in that they are just direct wrappers around container_of()... I'll
settle on something and switch everything over.

I'm down to about 200 more callsites. :) Found a missed pattern that
coccinelle could handle which caught about 1/3rd of my earlier
300ish...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1725851 — Re: Converting struct timer_list callback argument to struct timer_list *

On Sun, Sep 03, 2017 at 01:18:44PM -0700, Kees Cook wrote:
> It feels weird to have different semantics from container_of() too, so
> I'll probably just switch everything around to be like all the others,
> in that they are just direct wrappers around container_of()... I'll
> settle on something and switch everything over.

I really like the idea of passing in a variable and not the type,
though.  Maybe we need to add a new generic helper for that, I just
can't think of a good name.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web