* Converting struct timer_list callback argument to struct timer_list *
@ 2017-08-30 23:39 Kees Cook
2017-08-31 20:13 ` Thomas Gleixner
2017-09-01 10:08 ` Christoph Hellwig
0 siblings, 2 replies; 10+ messages in thread
From: Kees Cook @ 2017-08-30 23:39 UTC (permalink / raw)
To: Thomas Gleixner, Christoph Hellwig; +Cc: LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-08-30 23:39 Converting struct timer_list callback argument to struct timer_list * Kees Cook
@ 2017-08-31 20:13 ` Thomas Gleixner
2017-08-31 23:32 ` Kees Cook
2017-09-01 10:08 ` Christoph Hellwig
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2017-08-31 20:13 UTC (permalink / raw)
To: Kees Cook; +Cc: Christoph Hellwig, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-08-31 20:13 ` Thomas Gleixner
@ 2017-08-31 23:32 ` Kees Cook
0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2017-08-31 23:32 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Christoph Hellwig, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-08-30 23:39 Converting struct timer_list callback argument to struct timer_list * Kees Cook
2017-08-31 20:13 ` Thomas Gleixner
@ 2017-09-01 10:08 ` Christoph Hellwig
2017-09-01 10:21 ` Thomas Gleixner
1 sibling, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-09-01 10:08 UTC (permalink / raw)
To: Kees Cook; +Cc: Thomas Gleixner, Christoph Hellwig, LKML
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-09-01 10:08 ` Christoph Hellwig
@ 2017-09-01 10:21 ` Thomas Gleixner
2017-09-01 10:44 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2017-09-01 10:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Kees Cook, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-09-01 10:21 ` Thomas Gleixner
@ 2017-09-01 10:44 ` Christoph Hellwig
2017-09-01 16:24 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-09-01 10:44 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Christoph Hellwig, Kees Cook, LKML
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-09-01 10:44 ` Christoph Hellwig
@ 2017-09-01 16:24 ` Kees Cook
2017-09-03 7:37 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2017-09-01 16:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Thomas Gleixner, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-09-01 16:24 ` Kees Cook
@ 2017-09-03 7:37 ` Christoph Hellwig
2017-09-03 20:18 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-09-03 7:37 UTC (permalink / raw)
To: Kees Cook; +Cc: Christoph Hellwig, Thomas Gleixner, LKML
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-09-03 7:37 ` Christoph Hellwig
@ 2017-09-03 20:18 ` Kees Cook
2017-09-04 6:07 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2017-09-03 20:18 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Thomas Gleixner, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Converting struct timer_list callback argument to struct timer_list *
2017-09-03 20:18 ` Kees Cook
@ 2017-09-04 6:07 ` Christoph Hellwig
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2017-09-04 6:07 UTC (permalink / raw)
To: Kees Cook; +Cc: Christoph Hellwig, Thomas Gleixner, LKML
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-04 6:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-30 23:39 Converting struct timer_list callback argument to struct timer_list * Kees Cook
2017-08-31 20:13 ` Thomas Gleixner
2017-08-31 23:32 ` Kees Cook
2017-09-01 10:08 ` Christoph Hellwig
2017-09-01 10:21 ` Thomas Gleixner
2017-09-01 10:44 ` Christoph Hellwig
2017-09-01 16:24 ` Kees Cook
2017-09-03 7:37 ` Christoph Hellwig
2017-09-03 20:18 ` Kees Cook
2017-09-04 6:07 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).