| Message ID | 20171005193118.GA105874@beast |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] isdn/gigaset: Convert timers to use timer_setup() | expand |
On Thu, 2017-10-05 at 12:31 -0700, Kees Cook wrote: > --- a/drivers/isdn/gigaset/bas-gigaset.c > +++ b/drivers/isdn/gigaset/bas-gigaset.c > -static void cmd_in_timeout(unsigned long data) > +static void cmd_in_timeout(struct timer_list *t) > { > - struct cardstate *cs = (struct cardstate *) data; > - struct bas_cardstate *ucs = cs->hw.bas; > + struct bas_cardstate *ucs = from_timer(ucs, t, timer_cmd_in); > + struct urb *urb = ucs->urb_int_in; > + struct cardstate *cs = urb->context; This makes me nervous. Are you sure urb->context points to a struct cardstate here and in the other two places this patch changes? Anyhow, I'd like to have some time to do my review. So what's your timeframe here? I do hope I have at least a few weeks. (In other words: I hope gigaset isn't the only driver where the ability to use random pointers in these timer callbacks is removed.) Thanks, Paul Bolle
On Fri, Oct 6, 2017 at 12:00 PM, Paul Bolle <pebolle@tiscali.nl> wrote: > On Thu, 2017-10-05 at 12:31 -0700, Kees Cook wrote: >> --- a/drivers/isdn/gigaset/bas-gigaset.c >> +++ b/drivers/isdn/gigaset/bas-gigaset.c > >> -static void cmd_in_timeout(unsigned long data) >> +static void cmd_in_timeout(struct timer_list *t) >> { >> - struct cardstate *cs = (struct cardstate *) data; >> - struct bas_cardstate *ucs = cs->hw.bas; >> + struct bas_cardstate *ucs = from_timer(ucs, t, timer_cmd_in); >> + struct urb *urb = ucs->urb_int_in; >> + struct cardstate *cs = urb->context; > > This makes me nervous. Are you sure urb->context points to a struct cardstate > here and in the other two places this patch changes? > > Anyhow, I'd like to have some time to do my review. So what's your timeframe > here? I do hope I have at least a few weeks. (In other words: I hope gigaset > isn't the only driver where the ability to use random pointers in these timer > callbacks is removed.) Hi! I'm in no rush for any specific change. There are about 900 call sites I'm making my way through, about 2/3rd are pretty trivial, and the less obvious is what I've started sending out now, since I expect some will need some more careful review. Thanks for taking a look at it! -Kees
From: Kees Cook > Sent: 06 October 2017 20:40 ... > I'm in no rush for any specific change. There are about 900 call sites > I'm making my way through, about 2/3rd are pretty trivial, and the > less obvious is what I've started sending out now, since I expect some > will need some more careful review. Is it worth adding a structure that contains a timer and an extra 'long' than can be used to maintain the existing API logic for the 'difficult' cases? David
On Mon, Oct 9, 2017 at 2:15 AM, David Laight <David.Laight@aculab.com> wrote: > From: Kees Cook >> Sent: 06 October 2017 20:40 > ... >> I'm in no rush for any specific change. There are about 900 call sites >> I'm making my way through, about 2/3rd are pretty trivial, and the >> less obvious is what I've started sending out now, since I expect some >> will need some more careful review. > > Is it worth adding a structure that contains a timer and an extra 'long' > than can be used to maintain the existing API logic for the 'difficult' > cases? I didn't want to have this available in the general case, since I'd like to get all the conversions actually finished. There are a couple very special cases that need this, and they have one-off structs that do this. -Kees
diff --git a/drivers/isdn/gigaset/bas-gigaset.c b/drivers/isdn/gigaset/bas-gigaset.c index 33151f05e744..c990c6bbffc2 100644 --- a/drivers/isdn/gigaset/bas-gigaset.c +++ b/drivers/isdn/gigaset/bas-gigaset.c @@ -433,10 +433,11 @@ static void check_pending(struct bas_cardstate *ucs) * argument: * controller state structure */ -static void cmd_in_timeout(unsigned long data) +static void cmd_in_timeout(struct timer_list *t) { - struct cardstate *cs = (struct cardstate *) data; - struct bas_cardstate *ucs = cs->hw.bas; + struct bas_cardstate *ucs = from_timer(ucs, t, timer_cmd_in); + struct urb *urb = ucs->urb_int_in; + struct cardstate *cs = urb->context; int rc; if (!ucs->rcvbuf_size) { @@ -639,10 +640,11 @@ static void int_in_work(struct work_struct *work) * argument: * controller state structure */ -static void int_in_resubmit(unsigned long data) +static void int_in_resubmit(struct timer_list *t) { - struct cardstate *cs = (struct cardstate *) data; - struct bas_cardstate *ucs = cs->hw.bas; + struct bas_cardstate *ucs = from_timer(ucs, t, timer_int_in); + struct urb *urb = ucs->urb_int_in; + struct cardstate *cs = urb->context; int rc; if (ucs->retry_int_in++ >= BAS_RETRY) { @@ -1441,10 +1443,11 @@ static void read_iso_tasklet(unsigned long data) * argument: * controller state structure */ -static void req_timeout(unsigned long data) +static void req_timeout(struct timer_list *t) { - struct cardstate *cs = (struct cardstate *) data; - struct bas_cardstate *ucs = cs->hw.bas; + struct bas_cardstate *ucs = from_timer(ucs, t, timer_ctrl); + struct urb *urb = ucs->urb_int_in; + struct cardstate *cs = urb->context; int pending; unsigned long flags; @@ -1837,10 +1840,11 @@ static void write_command_callback(struct urb *urb) * argument: * controller state structure */ -static void atrdy_timeout(unsigned long data) +static void atrdy_timeout(struct timer_list *t) { - struct cardstate *cs = (struct cardstate *) data; - struct bas_cardstate *ucs = cs->hw.bas; + struct bas_cardstate *ucs = from_timer(ucs, t, timer_atrdy); + struct urb *urb = ucs->urb_int_in; + struct cardstate *cs = urb->context; dev_warn(cs->dev, "timeout waiting for HD_READY_SEND_ATDATA\n"); @@ -2213,10 +2217,10 @@ static int gigaset_initcshw(struct cardstate *cs) } spin_lock_init(&ucs->lock); - 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); init_waitqueue_head(&ucs->waitqueue); INIT_WORK(&ucs->int_in_wq, int_in_work);
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. Cc: Paul Bolle <pebolle@tiscali.nl> Cc: Karsten Keil <isdn@linux-pingi.de> Cc: "David S. Miller" <davem@davemloft.net> Cc: Johan Hovold <johan@kernel.org> Cc: gigaset307x-common@lists.sourceforge.net Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- This requires commit 686fef928bba ("timer: Prepare to change timer callback argument type") in v4.14-rc3, but should be otherwise stand-alone. v2: - split kzalloc() into a separate patch; pebolle. --- drivers/isdn/gigaset/bas-gigaset.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-)