Paged RX skb patch broke the defragmentation. We need to read hdr again
after linearization.
It fixes following bug
http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
Signed-off-by: Zhu, Yi <[email protected]>
Signed-off-by: Abhijeet Kolekar <[email protected]>
---
v2: Changed hdr reading.
v3: Added more comments.
net/mac80211/rx.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 9a08f2c..6e2a7bc 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
if (skb_linearize(rx->skb))
return RX_DROP_UNUSABLE;
+ /*
+ * skb_linearize() might change the skb->data and
+ * previously cached variables (in this case, hdr) need to
+ * be refreshed with the new data.
+ */
+ hdr = (struct ieee80211_hdr *)rx->skb->data;
seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
if (frag == 0) {
--
1.6.3.3
Hello John,
On Tue, 2010-05-11 at 11:14 -0700, John W. Linville wrote:
> On Tue, May 11, 2010 at 11:22:11AM -0700, Abhijeet Kolekar wrote:
> > Paged RX skb patch broke the defragmentation. We need to read hdr again
> > after linearization.
> >
> > It fixes following bug
> > http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
> >
> > Signed-off-by: Zhu, Yi <[email protected]>
> > Signed-off-by: Abhijeet Kolekar <[email protected]>
> > ---
> > v2: Changed hdr reading.
> > v3: Added more comments.
> > net/mac80211/rx.c | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 9a08f2c..6e2a7bc 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> > if (skb_linearize(rx->skb))
> > return RX_DROP_UNUSABLE;
> >
> > + /*
> > + * skb_linearize() might change the skb->data and
> > + * previously cached variables (in this case, hdr) need to
> > + * be refreshed with the new data.
> > + */
> > + hdr = (struct ieee80211_hdr *)rx->skb->data;
> > seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
> >
> > if (frag == 0) {
>
> And what about making sure the compiler doesn't optimize this away?
>
To avoid the double assignment, there is one more approach is to
directly read fc and seq_ctrl using skb_data. I will send that in the
next version.
Abhijeet
> John
On Tue, May 11, 2010 at 09:04:56PM +0200, Johannes Berg wrote:
> On Tue, 2010-05-11 at 11:52 -0700, Abhijeet Kolekar wrote:
> > Hello John,
> > On Tue, 2010-05-11 at 11:24 -0700, John W. Linville wrote:
> > > On Tue, May 11, 2010 at 11:16:50AM -0700, Abhijeet Kolekar wrote:
> > > > Hello John,
> > > > On Tue, 2010-05-11 at 11:14 -0700, John W. Linville wrote:
> > > > > On Tue, May 11, 2010 at 11:22:11AM -0700, Abhijeet Kolekar wrote:
> > > > > > Paged RX skb patch broke the defragmentation. We need to read hdr again
> > > > > > after linearization.
> > > > > >
> > > > > > It fixes following bug
> > > > > > http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
> > > > > >
> > > > > > Signed-off-by: Zhu, Yi <[email protected]>
> > > > > > Signed-off-by: Abhijeet Kolekar <[email protected]>
> > > > > > ---
> > > > > > v2: Changed hdr reading.
> > > > > > v3: Added more comments.
> > > > > > net/mac80211/rx.c | 6 ++++++
> > > > > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > > > > >
> > > > > > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > > > > > index 9a08f2c..6e2a7bc 100644
> > > > > > --- a/net/mac80211/rx.c
> > > > > > +++ b/net/mac80211/rx.c
> > > > > > @@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> > > > > > if (skb_linearize(rx->skb))
> > > > > > return RX_DROP_UNUSABLE;
> > > > > >
> > > > > > + /*
> > > > > > + * skb_linearize() might change the skb->data and
> > > > > > + * previously cached variables (in this case, hdr) need to
> > > > > > + * be refreshed with the new data.
> > > > > > + */
> > > > > > + hdr = (struct ieee80211_hdr *)rx->skb->data;
> > > > > > seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
> > > > > >
> > > > > > if (frag == 0) {
> > > > >
> > > > > And what about making sure the compiler doesn't optimize this away?
> > > > >
> > > > To avoid the double assignment, there is one more approach is to
> > > > directly read fc and seq_ctrl using skb_data. I will send that in the
> > > > next version.
> > >
> > > I don't think the double assignment is so bad, I just think that a
> > > compiler might decide to ignore the second assignment. Am I wrong?
> > >
> > I don't understand why compiler will ignore the second assignment other
> > than the above reason. What will be the solution in this case?
>
> ACCESS_ONCE()? I have no idea why/if the compiler would actually do this
> though.
I don't know about "if", but "why" might be that as far as the compiler
can see you have two identical assignments without an obvious change to
the data source in between. But maybe passing rx->skb to skb_linearize
is enough information to make the compiler aware that rx->skb->data
might have changed? Any better language lawyers than me around?
I think ACCESS_ONCE would be enough, but maybe it isn't necessary...?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Tue, 2010-05-11 at 11:52 -0700, Abhijeet Kolekar wrote:
> Hello John,
> On Tue, 2010-05-11 at 11:24 -0700, John W. Linville wrote:
> > On Tue, May 11, 2010 at 11:16:50AM -0700, Abhijeet Kolekar wrote:
> > > Hello John,
> > > On Tue, 2010-05-11 at 11:14 -0700, John W. Linville wrote:
> > > > On Tue, May 11, 2010 at 11:22:11AM -0700, Abhijeet Kolekar wrote:
> > > > > Paged RX skb patch broke the defragmentation. We need to read hdr again
> > > > > after linearization.
> > > > >
> > > > > It fixes following bug
> > > > > http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
> > > > >
> > > > > Signed-off-by: Zhu, Yi <[email protected]>
> > > > > Signed-off-by: Abhijeet Kolekar <[email protected]>
> > > > > ---
> > > > > v2: Changed hdr reading.
> > > > > v3: Added more comments.
> > > > > net/mac80211/rx.c | 6 ++++++
> > > > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > > > > index 9a08f2c..6e2a7bc 100644
> > > > > --- a/net/mac80211/rx.c
> > > > > +++ b/net/mac80211/rx.c
> > > > > @@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> > > > > if (skb_linearize(rx->skb))
> > > > > return RX_DROP_UNUSABLE;
> > > > >
> > > > > + /*
> > > > > + * skb_linearize() might change the skb->data and
> > > > > + * previously cached variables (in this case, hdr) need to
> > > > > + * be refreshed with the new data.
> > > > > + */
> > > > > + hdr = (struct ieee80211_hdr *)rx->skb->data;
> > > > > seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
> > > > >
> > > > > if (frag == 0) {
> > > >
> > > > And what about making sure the compiler doesn't optimize this away?
> > > >
> > > To avoid the double assignment, there is one more approach is to
> > > directly read fc and seq_ctrl using skb_data. I will send that in the
> > > next version.
> >
> > I don't think the double assignment is so bad, I just think that a
> > compiler might decide to ignore the second assignment. Am I wrong?
> >
> I don't understand why compiler will ignore the second assignment other
> than the above reason. What will be the solution in this case?
ACCESS_ONCE()? I have no idea why/if the compiler would actually do this
though.
johannes
On Tue, May 11, 2010 at 11:16:50AM -0700, Abhijeet Kolekar wrote:
> Hello John,
> On Tue, 2010-05-11 at 11:14 -0700, John W. Linville wrote:
> > On Tue, May 11, 2010 at 11:22:11AM -0700, Abhijeet Kolekar wrote:
> > > Paged RX skb patch broke the defragmentation. We need to read hdr again
> > > after linearization.
> > >
> > > It fixes following bug
> > > http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
> > >
> > > Signed-off-by: Zhu, Yi <[email protected]>
> > > Signed-off-by: Abhijeet Kolekar <[email protected]>
> > > ---
> > > v2: Changed hdr reading.
> > > v3: Added more comments.
> > > net/mac80211/rx.c | 6 ++++++
> > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > > index 9a08f2c..6e2a7bc 100644
> > > --- a/net/mac80211/rx.c
> > > +++ b/net/mac80211/rx.c
> > > @@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> > > if (skb_linearize(rx->skb))
> > > return RX_DROP_UNUSABLE;
> > >
> > > + /*
> > > + * skb_linearize() might change the skb->data and
> > > + * previously cached variables (in this case, hdr) need to
> > > + * be refreshed with the new data.
> > > + */
> > > + hdr = (struct ieee80211_hdr *)rx->skb->data;
> > > seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
> > >
> > > if (frag == 0) {
> >
> > And what about making sure the compiler doesn't optimize this away?
> >
> To avoid the double assignment, there is one more approach is to
> directly read fc and seq_ctrl using skb_data. I will send that in the
> next version.
I don't think the double assignment is so bad, I just think that a
compiler might decide to ignore the second assignment. Am I wrong?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Tue, May 11, 2010 at 11:22:11AM -0700, Abhijeet Kolekar wrote:
> Paged RX skb patch broke the defragmentation. We need to read hdr again
> after linearization.
>
> It fixes following bug
> http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
>
> Signed-off-by: Zhu, Yi <[email protected]>
> Signed-off-by: Abhijeet Kolekar <[email protected]>
> ---
> v2: Changed hdr reading.
> v3: Added more comments.
> net/mac80211/rx.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 9a08f2c..6e2a7bc 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> if (skb_linearize(rx->skb))
> return RX_DROP_UNUSABLE;
>
> + /*
> + * skb_linearize() might change the skb->data and
> + * previously cached variables (in this case, hdr) need to
> + * be refreshed with the new data.
> + */
> + hdr = (struct ieee80211_hdr *)rx->skb->data;
> seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
>
> if (frag == 0) {
And what about making sure the compiler doesn't optimize this away?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
Hello John,
On Tue, 2010-05-11 at 11:24 -0700, John W. Linville wrote:
> On Tue, May 11, 2010 at 11:16:50AM -0700, Abhijeet Kolekar wrote:
> > Hello John,
> > On Tue, 2010-05-11 at 11:14 -0700, John W. Linville wrote:
> > > On Tue, May 11, 2010 at 11:22:11AM -0700, Abhijeet Kolekar wrote:
> > > > Paged RX skb patch broke the defragmentation. We need to read hdr again
> > > > after linearization.
> > > >
> > > > It fixes following bug
> > > > http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2194
> > > >
> > > > Signed-off-by: Zhu, Yi <[email protected]>
> > > > Signed-off-by: Abhijeet Kolekar <[email protected]>
> > > > ---
> > > > v2: Changed hdr reading.
> > > > v3: Added more comments.
> > > > net/mac80211/rx.c | 6 ++++++
> > > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > > > index 9a08f2c..6e2a7bc 100644
> > > > --- a/net/mac80211/rx.c
> > > > +++ b/net/mac80211/rx.c
> > > > @@ -1253,6 +1253,12 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> > > > if (skb_linearize(rx->skb))
> > > > return RX_DROP_UNUSABLE;
> > > >
> > > > + /*
> > > > + * skb_linearize() might change the skb->data and
> > > > + * previously cached variables (in this case, hdr) need to
> > > > + * be refreshed with the new data.
> > > > + */
> > > > + hdr = (struct ieee80211_hdr *)rx->skb->data;
> > > > seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
> > > >
> > > > if (frag == 0) {
> > >
> > > And what about making sure the compiler doesn't optimize this away?
> > >
> > To avoid the double assignment, there is one more approach is to
> > directly read fc and seq_ctrl using skb_data. I will send that in the
> > next version.
>
> I don't think the double assignment is so bad, I just think that a
> compiler might decide to ignore the second assignment. Am I wrong?
>
I don't understand why compiler will ignore the second assignment other
than the above reason. What will be the solution in this case?
Abhijeet
> John