linux-kernel - Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080306104034.GB27894@ZenIV.linux.org.uk>
Date:	Thu, 6 Mar 2008 10:40:35 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Jeff Garzik <jeff@...zik.org>
Subject: Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

On Wed, Mar 05, 2008 at 01:55:36PM +1100, Rusty Russell wrote:

> AFAICT you posted an untested code fragment which won't actually compile 
> because the callback in this case returns void.

Callbacks for timers *do* return void.  FWIW,

extern void decay_to_pointer(void const volatile *);
#define check_callback_type(f,x) (sizeof((0 ? decay_to_pointer(x),(f)(x) : (void)0), 0))
#define __setup_timer(timer, func, arg) \
do {    \
	struct timer_list *__timer = (timer);   \
	__timer->function = (void (*)(unsigned long))(func); \
	__timer->data = (unsigned long)(arg);   \
	(void)check_callback_type(func, arg); \
} while(0)

#define setup_timer(timer, func, arg) \
do {    \
	struct timer_list *__timer = (timer);   \
	__timer->function = (void (*)(unsigned long))(func); \
	__timer->data = (unsigned long)(arg);   \
	init_timer(__timer);    \
	(void)check_callback_type(func, arg); \
} while(0)

#define DECLARE_TIMER(_name, _function, _data) \
        struct timer_list _name = { \
                .base = &boot_tvec_bases, \
                .function = (void (*)(unsigned long))(_function), \
                .data = (unsigned long)(_data) + \
                        0 * check_callback_type(_function, _data) \
        }

is what the timer series ended up using.  Note that this is _after_
conversion of setup_timer() users to natural argument types.  And yes,
it went timer-by-timer.

> > > ; normal C constructs are quite sufficient, TYVM.
> 
> Your code didn't check the return type, except that it's a pointer or 
> int-compatible.

Huh?  It certainly does _not_ accept return of int or pointer - ? : with
(void) 0 in the other branch is there to give a warning on those.  Note
that any timer callback that does anything of that kind is simply bogus -
there is nothing the caller could do about whatever return values we might
have, anyway.

> We can do better, as this patch showed.

> If you have a "saner way" of doing this in general which is just as effective, 
> I look forward to it.  Otherwise you're just managed to delay these 
> improvements for another release.

See above.  The point is, we don't bloody need to convert them all at once
and we can eliminate void (unsigned long) ones as we go.

Let me put it that way: if you actually look at these suckers, you'll see
that there's less than a dozen instances that actually want an integer.
Out of several hundreds.  And out of those only two, IIRC, were not trivially
converted by "pass pointer to array element instead of index in array".
Something in arch/sparc and arch/sparc64 - I can dig the details out if
you wish, but (a) it'll obviously need search from scratch to verify that
new pathology didn't show up and (b) it's extremely unlikely that the total
amount had become considerable.

So for timer callbacks the right answer is "just convert them to pointers,
these two cases are not worth the trouble".  And we _can_ do that in bisectable
way, TYVM - it's not that we can't do conversion stepwise.

A _very_ long series (we've a lot of these suckers) and I had two of its
incarnations bitrot on me by now, but it's not particulary tricky.

FWIW, I've finally rescued another bitrot victim (bdev API conversion and
related cleanups and fixes; sent to Jens for review last week, might be
actually mergable next cycle), so the timer one is near the top of the pile
right now; I can certainly bump it up and post the infrastructure + several
hundred instances converted by the next week.  Finishing vfs-2.6 tree should
take a couple of days (there are several _old_ pending mini-series that might
go there; currently the tree on kernel.org still has only immediate fixes +
ro-bind, all under vfs-fixes.b1 / ro-bind.b1), then I'm free for that...

PS: apologies for missing your previous mail; ETOOBIGMBOX ;-/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/