[PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[Kees Cook]

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(-)

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);
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[Paul Bolle]

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

Re: [PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

RE: [PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[David Laight]

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

RE: [PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[David Laight]

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

Re: [PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] isdn/gigaset: Convert timers to use timer_setup()

[Kees Cook]

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

-- 
Kees Cook
Pixel Security