Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote:

On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
[off-list ref] wrote:

quoted

quoted

This has been the least trivial timer conversion yet. Given the use of
RCU and other things I may not even know about, I'd love to get a close
look at this. I *think* this is correct, as it will re-lookup the tid
entries when firing the timer.

I'm not really sure why you're doing the lookup again? That seems
pointless, since you already have the right structure, and already rely
on it being valid. You can't really get a new struct assigned to the
same TID without the old one being destroyed.

I couldn't tell what the lifetime expectation was, so I left the
re-lookup. I assumed it was possible to have a timer fire after the
structure had been removed from the station structure.

Oh, right, I guess that would've been possible in theory. It's not
actually possible though since the aggregation sessions can't live on
without a station, so I've already made a follow-up patch to remove the
indirection.

johannes

On Wed, Oct 18, 2017 at 1:50 PM, Johannes Berg
[off-list ref] wrote:

On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote:

quoted

On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
[off-list ref] wrote:

quoted

quoted

This has been the least trivial timer conversion yet. Given the use of
RCU and other things I may not even know about, I'd love to get a close
look at this. I *think* this is correct, as it will re-lookup the tid
entries when firing the timer.

I'm not really sure why you're doing the lookup again? That seems
pointless, since you already have the right structure, and already rely
on it being valid. You can't really get a new struct assigned to the
same TID without the old one being destroyed.

I couldn't tell what the lifetime expectation was, so I left the
re-lookup. I assumed it was possible to have a timer fire after the
structure had been removed from the station structure.

Oh, right, I guess that would've been possible in theory. It's not
actually possible though since the aggregation sessions can't live on
without a station, so I've already made a follow-up patch to remove the
indirection.

Okay, cool, thanks.

It seems like I have a similar case in the iwlwifi driver too, but
it's different enough that I'm not sure about that one either. I'll
send that when I've got it ready.

-Kees

-- 
Kees Cook
Pixel Security