[PATCH net-next] mac80211: Check correct skb for shared states before freeing original

[PATCH net-next] mac80211: Check correct skb for shared states before freeing original

[Alexander Duyck]

The code for cloning the skb for an acknowledgement was checking to see if
the cloned skb was shared and if it was it was then freeing the original
skb.  Since a clone should never really be shared I suspect that the
intention was to avoid freeing the clone if the original was shared.  As
such I am updating the code so that if the original is shared we free the
original and use the clone.  This avoids unnecessary work in the next
section where we would be cloning the skb if the original is shared.

Signed-off-by: Alexander Duyck <alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 net/mac80211/tx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 925c39f..e527cd3 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 			if (id >= 0) {
 				info_id = id;
 				info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
-			} else if (skb_shared(skb)) {
+			} else if (skb_shared(orig_skb)) {
 				kfree_skb(orig_skb);
 			} else {
 				kfree_skb(skb);

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] mac80211: Check correct skb for shared states before freeing original

[Johannes Berg]

On Wed, 2014-09-10 at 16:06 -0400, Alexander Duyck wrote:
> The code for cloning the skb for an acknowledgement was checking to see if
> the cloned skb was shared and if it was it was then freeing the original
> skb.  Since a clone should never really be shared I suspect that the
> intention was to avoid freeing the clone if the original was shared.  As
> such I am updating the code so that if the original is shared we free the
> original and use the clone.  This avoids unnecessary work in the next
> section where we would be cloning the skb if the original is shared.

Thanks, yeah, I admit that this is clearly fishy.

> @@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  			if (id >= 0) {
>  				info_id = id;
>  				info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;

Luckily, we practically always go into this path.

> -			} else if (skb_shared(skb)) {
> +			} else if (skb_shared(orig_skb)) {
>  				kfree_skb(orig_skb);
>  			} else {
>  				kfree_skb(skb);

We have a clone already so we could just remove the whole "else if" I
think, but I'm guessing my intent was to keep it accounted to the socket
where possible rather than freeing the original in all cases.

So yeah, I think this makes sense. Maybe we should add a comment to the
if though to explain this?

johannes

Re: [PATCH net-next] mac80211: Check correct skb for shared states before freeing original

[Alexander Duyck]

On 09/10/2014 01:33 PM, Johannes Berg wrote:
> On Wed, 2014-09-10 at 16:06 -0400, Alexander Duyck wrote:
>> The code for cloning the skb for an acknowledgement was checking to see if
>> the cloned skb was shared and if it was it was then freeing the original
>> skb.  Since a clone should never really be shared I suspect that the
>> intention was to avoid freeing the clone if the original was shared.  As
>> such I am updating the code so that if the original is shared we free the
>> original and use the clone.  This avoids unnecessary work in the next
>> section where we would be cloning the skb if the original is shared.
> 
> Thanks, yeah, I admit that this is clearly fishy.
> 
>> @@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  			if (id >= 0) {
>>  				info_id = id;
>>  				info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
> 
> Luckily, we practically always go into this path.
> 
>> -			} else if (skb_shared(skb)) {
>> +			} else if (skb_shared(orig_skb)) {
>>  				kfree_skb(orig_skb);
>>  			} else {
>>  				kfree_skb(skb);
> 
> We have a clone already so we could just remove the whole "else if" I
> think, but I'm guessing my intent was to keep it accounted to the socket
> where possible rather than freeing the original in all cases.
> 
> So yeah, I think this makes sense. Maybe we should add a comment to the
> if though to explain this?
> 
> johannes

Actually I think we may need to take a different approach.  The reason I
was in this code was to take a look at a possible refcount issue.

I'll be submitting another patch in a few minutes and will probably be
dropping some of this code anyway.

Thanks,

Alex