[PATCH] ath9k: fix another source of corrupt frames

[PATCH] ath9k: fix another source of corrupt frames

[Felix Fietkau]

Atheros hardware supports receiving frames that span multiple
descriptors and buffers. In this case, the rx status of every
descriptor except for the last one is invalid and may contain random
data. Because the driver does not support this, it needs to drop such
frames.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
--- a/drivers/net/wireless/ath/ath9k/common.c
+++ b/drivers/net/wireless/ath/ath9k/common.c
@@ -57,13 +57,19 @@ static bool ath9k_rx_accept(struct ath_c
 	 * rs_more indicates chained descriptors which can be used
 	 * to link buffers together for a sort of scatter-gather
 	 * operation.
-	 *
+	 * reject the frame, we don't support scatter-gather yet and
+	 * the frame is probably corrupt anyway
+	 */
+	if (rx_stats->rs_more)
+		return false;
+
+	/*
 	 * The rx_stats->rs_status will not be set until the end of the
 	 * chained descriptors so it can be ignored if rs_more is set. The
 	 * rs_more will be false at the last element of the chained
 	 * descriptors.
 	 */
-	if (!rx_stats->rs_more && rx_stats->rs_status != 0) {
+	if (rx_stats->rs_status != 0) {
 		if (rx_stats->rs_status & ATH9K_RXERR_CRC)
 			rxs->flag |= RX_FLAG_FAILED_FCS_CRC;
 		if (rx_stats->rs_status & ATH9K_RXERR_PHY)

Re: [PATCH] ath9k: fix another source of corrupt frames

[Luis R. Rodriguez]

On Tue, May 4, 2010 at 12:58 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> Atheros hardware supports receiving frames that span multiple
> descriptors and buffers. In this case, the rx status of every
> descriptor except for the last one is invalid and may contain random
> data. Because the driver does not support this, it needs to drop such
> frames.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
> --- a/drivers/net/wireless/ath/ath9k/common.c
> +++ b/drivers/net/wireless/ath/ath9k/common.c
> @@ -57,13 +57,19 @@ static bool ath9k_rx_accept(struct ath_c
>         * rs_more indicates chained descriptors which can be used
>         * to link buffers together for a sort of scatter-gather
>         * operation.
> -        *
> +        * reject the frame, we don't support scatter-gather yet and
> +        * the frame is probably corrupt anyway
> +        */
> +       if (rx_stats->rs_more)
> +               return false;

Actually this is required by ath9k_htc, it does process these, but
ath9k doesn't so this could be done within ath9k itself.

  Luis

Re: [PATCH] ath9k: fix another source of corrupt frames

[John W. Linville]

On Tue, May 04, 2010 at 11:09:32AM -0700, Luis R. Rodriguez wrote:
> On Tue, May 4, 2010 at 12:58 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> > Atheros hardware supports receiving frames that span multiple
> > descriptors and buffers. In this case, the rx status of every
> > descriptor except for the last one is invalid and may contain random
> > data. Because the driver does not support this, it needs to drop such
> > frames.
> >
> > Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> > ---
> > --- a/drivers/net/wireless/ath/ath9k/common.c
> > +++ b/drivers/net/wireless/ath/ath9k/common.c
> > @@ -57,13 +57,19 @@ static bool ath9k_rx_accept(struct ath_c
> >         * rs_more indicates chained descriptors which can be used
> >         * to link buffers together for a sort of scatter-gather
> >         * operation.
> > -        *
> > +        * reject the frame, we don't support scatter-gather yet and
> > +        * the frame is probably corrupt anyway
> > +        */
> > +       if (rx_stats->rs_more)
> > +               return false;
> 
> Actually this is required by ath9k_htc, it does process these, but
> ath9k doesn't so this could be done within ath9k itself.

I'm sorry, that is a bit unclear to me.  Are you NAKing the patch?
Or saying that the patch is fine but another patch is needed on top?

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] ath9k: fix another source of corrupt frames

[Luis R. Rodriguez]

On Tue, May 04, 2010 at 11:36:13AM -0700, John W. Linville wrote:
> On Tue, May 04, 2010 at 11:09:32AM -0700, Luis R. Rodriguez wrote:
> > On Tue, May 4, 2010 at 12:58 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> > > Atheros hardware supports receiving frames that span multiple
> > > descriptors and buffers. In this case, the rx status of every
> > > descriptor except for the last one is invalid and may contain random
> > > data. Because the driver does not support this, it needs to drop such
> > > frames.
> > >
> > > Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> > > ---
> > > --- a/drivers/net/wireless/ath/ath9k/common.c
> > > +++ b/drivers/net/wireless/ath/ath9k/common.c
> > > @@ -57,13 +57,19 @@ static bool ath9k_rx_accept(struct ath_c
> > >         * rs_more indicates chained descriptors which can be used
> > >         * to link buffers together for a sort of scatter-gather
> > >         * operation.
> > > -        *
> > > +        * reject the frame, we don't support scatter-gather yet and
> > > +        * the frame is probably corrupt anyway
> > > +        */
> > > +       if (rx_stats->rs_more)
> > > +               return false;
> > 
> > Actually this is required by ath9k_htc, it does process these, but
> > ath9k doesn't so this could be done within ath9k itself.
> 
> I'm sorry, that is a bit unclear to me.  Are you NAKing the patch?

Yes, I am NAK'ing it, I should have been clearer about that, sorry.
The reason is that ath9k_htc *depends* on rs_more stuff because the
USB frames it receives *are* split up and to RX them we decouple
them. With this patch you'd break ath9k_htc considerably.

> Or saying that the patch is fine but another patch is needed on top?

My suggestion is that this change can be made but only on the
ath9k RX path, instead of the comon path so a separate patch would
be needed.

  Luis

Re: [PATCH] ath9k: fix another source of corrupt frames

[Felix Fietkau]

On 2010-05-04 8:09 PM, Luis R. Rodriguez wrote:
> On Tue, May 4, 2010 at 12:58 AM, Felix Fietkau <nbd@openwrt.org> wrote:
>> Atheros hardware supports receiving frames that span multiple
>> descriptors and buffers. In this case, the rx status of every
>> descriptor except for the last one is invalid and may contain random
>> data. Because the driver does not support this, it needs to drop such
>> frames.
>>
>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>> ---
>> --- a/drivers/net/wireless/ath/ath9k/common.c
>> +++ b/drivers/net/wireless/ath/ath9k/common.c
>> @@ -57,13 +57,19 @@ static bool ath9k_rx_accept(struct ath_c
>>         * rs_more indicates chained descriptors which can be used
>>         * to link buffers together for a sort of scatter-gather
>>         * operation.
>> -        *
>> +        * reject the frame, we don't support scatter-gather yet and
>> +        * the frame is probably corrupt anyway
>> +        */
>> +       if (rx_stats->rs_more)
>> +               return false;
> 
> Actually this is required by ath9k_htc, it does process these, but
> ath9k doesn't so this could be done within ath9k itself.
I don't see any place in ath9k_htc that processes rs_more. And even if
it did, processing the rx status of a frame that has more descriptors
chained after it would be wrong, since the rx status is only valid for
the last frame of the descriptor chain.
I think my patch would work fine for ath9k_htc as well.

- Felix

Re: [PATCH] ath9k: fix another source of corrupt frames

[Luis R. Rodriguez]

On Tue, May 04, 2010 at 12:36:26PM -0700, Felix Fietkau wrote:
> On 2010-05-04 8:09 PM, Luis R. Rodriguez wrote:
> > On Tue, May 4, 2010 at 12:58 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> >> Atheros hardware supports receiving frames that span multiple
> >> descriptors and buffers. In this case, the rx status of every
> >> descriptor except for the last one is invalid and may contain random
> >> data. Because the driver does not support this, it needs to drop such
> >> frames.
> >>
> >> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> >> ---
> >> --- a/drivers/net/wireless/ath/ath9k/common.c
> >> +++ b/drivers/net/wireless/ath/ath9k/common.c
> >> @@ -57,13 +57,19 @@ static bool ath9k_rx_accept(struct ath_c
> >>         * rs_more indicates chained descriptors which can be used
> >>         * to link buffers together for a sort of scatter-gather
> >>         * operation.
> >> -        *
> >> +        * reject the frame, we don't support scatter-gather yet and
> >> +        * the frame is probably corrupt anyway
> >> +        */
> >> +       if (rx_stats->rs_more)
> >> +               return false;
> > 
> > Actually this is required by ath9k_htc, it does process these, but
> > ath9k doesn't so this could be done within ath9k itself.
> I don't see any place in ath9k_htc that processes rs_more.

Odd, when I worked on it, I had to use this, let me check with today's
code and get back to this thread.

> And even if
> it did, processing the rx status of a frame that has more descriptors
> chained after it would be wrong, since the rx status is only valid for
> the last frame of the descriptor chain.

This is true.

> I think my patch would work fine for ath9k_htc as well.

We should test just to be sure. Would hate to request for a revert
for something we could have just prevented with proper testing/review.
I can test this in a bit I think.

  Luis

Re: [PATCH] ath9k: fix another source of corrupt frames

[Luis R. Rodriguez]

On Tue, May 4, 2010 at 12:55 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> ath9k_rx_accept

So this guys is called only within ath9k_cmn_rx_skb_preprocess() and
that is only called by ath9k.. so yeah ath9k_htc no longer uses this
stuff. The patch is fine, please do merge John.

  Luis