Re: [PATCH V3] mac80211: fix paged defragmentation

Re: [PATCH V3] mac80211: fix paged defragmentation

[John W. Linville]

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 <yi.zhu@intel.com>
> Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> ---
> 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
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH V3] mac80211: fix paged defragmentation

[Abhijeet Kolekar]

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 <yi.zhu@intel.com>
> > Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> > ---
> > 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

[PATCH V3] mac80211: fix paged defragmentation

[Abhijeet Kolekar]

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 <yi.zhu@intel.com>
Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
---
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

Re: [PATCH V3] mac80211: fix paged defragmentation

[John W. Linville]

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 <yi.zhu@intel.com>
> > > Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> > > ---
> > > 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
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH V3] mac80211: fix paged defragmentation

[Abhijeet Kolekar]

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 <yi.zhu@intel.com>
> > > > Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> > > > ---
> > > > 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

Re: [PATCH V3] mac80211: fix paged defragmentation

[Johannes Berg]

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 <yi.zhu@intel.com>
> > > > > Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> > > > > ---
> > > > > 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

Re: [PATCH V3] mac80211: fix paged defragmentation

[John W. Linville]

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 <yi.zhu@intel.com>
> > > > > > Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> > > > > > ---
> > > > > > 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
linville@tuxdriver.com			might be all we have.  Be ready.