ath9k: fix another source of corrupt frames
PATCH 1/1 ath9k: fix another source of corrupt frames
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.
---
drivers/net/wireless/ath/ath9k/common.c +8 -2
@@ -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 )
Quoted 11 lines · 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 <[email protected]>---
drivers/net/wireless/ath/ath9k/common.c static bool ath9k_rx_accept(struct ath_c · line 64
Quoted 9 lines
drivers/net/wireless/ath/ath9k/common.c +5
-1@@ -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
Quoted 16 lines · Luis R. Rodriguez
Quoted 15 lines · 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 <[email protected]>---
drivers/net/wireless/ath/ath9k/common.c +0
-1@@ -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. - *
Quoted 8 lines
drivers/net/wireless/ath/ath9k/common.c static bool ath9k_rx_accept(struct ath_c · line 64
Quoted 5 lines
drivers/net/wireless/ath/ath9k/common.c +5
-0+* 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
Signature · 2 lines
John W. Linville Someday the world will need a hero, and you [email protected] might be all we have. Be ready.
Quoted 26 lines · John W. Linville
Quoted 16 lines · Luis R. Rodriguez
Quoted 15 lines · 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 <[email protected]>---
drivers/net/wireless/ath/ath9k/common.c +0
-1@@ -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. - *Quoted 8 lines
drivers/net/wireless/ath/ath9k/common.c static bool ath9k_rx_accept(struct ath_c · line 64
Quoted 5 lines
drivers/net/wireless/ath/ath9k/common.c +5
-0+* 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.
Quoted 1 line
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.
Quoted 1 line
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
Quoted 16 lines · Luis R. Rodriguez
Quoted 15 lines · 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 <[email protected]>---
drivers/net/wireless/ath/ath9k/common.c +0
-1@@ -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. -*
Quoted 8 lines
drivers/net/wireless/ath/ath9k/common.c static bool ath9k_rx_accept(struct ath_c · line 64
Quoted 5 lines
drivers/net/wireless/ath/ath9k/common.c +5
-0+* 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
Quoted 25 lines · Felix Fietkau
Quoted 16 lines · Luis R. Rodriguez
Quoted 15 lines · 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 <[email protected]>---
drivers/net/wireless/ath/ath9k/common.c +0
-1@@ -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. -* Quoted 8 lines
drivers/net/wireless/ath/ath9k/common.c static bool ath9k_rx_accept(struct ath_c · line 64
Quoted 5 lines
drivers/net/wireless/ath/ath9k/common.c +5
-0+* 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.
Quoted 1 line
I don't see any place in
ath9k_htcthat processesrs_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.
Quoted 4 lines
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.
Quoted 1 line
I think my patch would work fine for
ath9k_htcas 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
Quoted 1 line · Luis R. Rodriguez
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