linux-kernel - Re: [PATCH 32/58] isdn/gigaset: Convert timers to use timer

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1508451394.2274.49.camel@tiscali.nl>
Date:   Fri, 20 Oct 2017 00:16:34 +0200
From:   Paul Bolle <pebolle@...cali.nl>
To:     Kees Cook <keescook@...omium.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Karsten Keil <isdn@...ux-pingi.de>,
        Johan Hovold <johan@...nel.org>,
        gigaset307x-common@...ts.sourceforge.net,
        Network Development <netdev@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 32/58] isdn/gigaset: Convert timers to use timer_setup()

On Thu, 2017-10-19 at 14:31 -0700, Kees Cook wrote:
> What I did in many other non-trivial conversions was just add an
> explicit pointer back, since that's operationally identical to what
> struct timer_list was storing in its .data field.
> 
> i.e.
> 
> add:
> 
>   struct cardstate *cs;
> 
> to struct bas_cardstate, and then use this on timer entry:
> 
>        struct bas_cardstate *ucs = from_timer(ucs, t, $timer...);
>        struct cardstate *cs = ucs->cs;

That crossed my mind too. (Honestly!) It _feels_ a bit dirty, as I have this
idea that structures having references to each other is some sort of an anti-
pattern. On the other hand: the various structures used here are, well, not
that clean already so I might as well ignore my feelings.

> and this at init:
> 
>         spin_lock_init(&ucs->lock);
> +      ucs->cs = cs;
> -       setup_timer(&ucs->timer_ctrl, req_timeout, (unsigned long) cs);
> -       setup_timer(&ucs->timer_atrdy, atrdy_timeout, (unsigned long) cs);
> -       setup_timer(&ucs->timer_cmd_in, cmd_in_timeout, (unsigned long) cs);
> -       setup_timer(&ucs->timer_int_in, int_in_resubmit, (unsigned long) cs);
> +       timer_setup(&ucs->timer_ctrl, req_timeout, 0);
> +       timer_setup(&ucs->timer_atrdy, atrdy_timeout, 0);
> +       timer_setup(&ucs->timer_cmd_in, cmd_in_timeout, 0);
> +       timer_setup(&ucs->timer_int_in, int_in_resubmit, 0);
> 
> which will avoid the urb entirely.
> 
> Do you want me to send an alternative patch?

That would be nice! Please allow a few days for testing.

That testing is beyond silly, though. It requires me getting a laptop very
close to the awkward place where my ISDN setup lives. I'll spare you the
details. But that silliness again shows, I'd say, that the gigaset code mainly
exists to see if there's still a pulse in mainline ISDN. Is that enough to
bother? Or should mainline ISDN go the way of, say, IRDA?

But I digress. An alternative patch would be much appreciated.

Thanks,


Paul Bolle