[PATCH v2] brcmfmac: Make skb header writable before use

[PATCH v2] brcmfmac: Make skb header writable before use

[James Hughes]

The driver was making changes to the skb_header without
ensuring it was writable (i.e. uncloned).
This patch also removes some boiler plate header size
checking/adjustment code as that is also handled by the
skb_cow_header function used to make header writable.

This patch depends on
brcmfmac: Ensure pointer correctly set if skb data location changes

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
Changes in v2
  Makes the _cow_ call at the entry point of the skb in to the
  stack, means only needs to be done once, and error handling
  is easier.
  Split a separate minor bug fix off to a separate patch (which
  this patch depends on)



 .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 9b7c19a508ac..88f8675a94c2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 		goto done;
 	}
 
-	/* Make sure there's enough room for any header */
-	if (skb_headroom(skb) < drvr->hdrlen) {
-		struct sk_buff *skb2;
-
-		brcmf_dbg(INFO, "%s: insufficient headroom\n",
-			  brcmf_ifname(ifp));
-		drvr->bus_if->tx_realloc++;
-		skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
+	/* Make sure there's enough room for any header, and make it writable */
+	if (skb_cow_head(skb, drvr->hdrlen)) {
 		dev_kfree_skb(skb);
-		skb = skb2;
-		if (skb == NULL) {
-			brcmf_err("%s: skb_realloc_headroom failed\n",
-				  brcmf_ifname(ifp));
-			ret = -ENOMEM;
-			goto done;
-		}
+		ret = -ENOMEM;
+		goto done;
 	}
 
 	/* validate length for ether packet */
-- 
2.11.0

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Eric Dumazet]

On Mon, 2017-04-24 at 14:03 +0100, James Hughes wrote:
> The driver was making changes to the skb_header without
> ensuring it was writable (i.e. uncloned).
> This patch also removes some boiler plate header size
> checking/adjustment code as that is also handled by the
> skb_cow_header function used to make header writable.
> 
> This patch depends on
> brcmfmac: Ensure pointer correctly set if skb data location changes
> 
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
> Changes in v2
>   Makes the _cow_ call at the entry point of the skb in to the
>   stack, means only needs to be done once, and error handling
>   is easier.
>   Split a separate minor bug fix off to a separate patch (which
>   this patch depends on)

Best way to handle dependencies is to send a patch series.

1/2 brcmfmac: Ensure pointer correctly set if skb data location changes
2/2 brcmfmac: Make skb header writable before use

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 24-4-2017 20:09, Eric Dumazet wrote:
> On Mon, 2017-04-24 at 14:03 +0100, James Hughes wrote:
>> The driver was making changes to the skb_header without
>> ensuring it was writable (i.e. uncloned).
>> This patch also removes some boiler plate header size
>> checking/adjustment code as that is also handled by the
>> skb_cow_header function used to make header writable.
>>
>> This patch depends on
>> brcmfmac: Ensure pointer correctly set if skb data location changes
>>
>> Signed-off-by: James Hughes <james.hughes-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>> ---
>> Changes in v2
>>   Makes the _cow_ call at the entry point of the skb in to the
>>   stack, means only needs to be done once, and error handling
>>   is easier.
>>   Split a separate minor bug fix off to a separate patch (which
>>   this patch depends on)
> 
> Best way to handle dependencies is to send a patch series.
> 
> 1/2 brcmfmac: Ensure pointer correctly set if skb data location changes
> 2/2 brcmfmac: Make skb header writable before use

There is actually no real dependency. Both are valid fixes of course but
either one applies to wireless-drivers-next/master on its own.

Regards,
Arend

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 24-4-2017 15:03, James Hughes wrote:
> The driver was making changes to the skb_header without
> ensuring it was writable (i.e. uncloned).
> This patch also removes some boiler plate header size
> checking/adjustment code as that is also handled by the
> skb_cow_header function used to make header writable.
> 
> This patch depends on
> brcmfmac: Ensure pointer correctly set if skb data location changes

Hi James,

I actually thought I was going to tackle this so I spend some time
working on it today, but I will submit that as a subsequent patch. Below
some comments but apart from that:

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
> Changes in v2
>   Makes the _cow_ call at the entry point of the skb in to the
>   stack, means only needs to be done once, and error handling
>   is easier.
>   Split a separate minor bug fix off to a separate patch (which
>   this patch depends on)
> 
> 
> 
>  .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9b7c19a508ac..88f8675a94c2 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>  		goto done;
>  	}
>  
> -	/* Make sure there's enough room for any header */
> -	if (skb_headroom(skb) < drvr->hdrlen) {
> -		struct sk_buff *skb2;
> -
> -		brcmf_dbg(INFO, "%s: insufficient headroom\n",
> -			  brcmf_ifname(ifp));
> -		drvr->bus_if->tx_realloc++;
> -		skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
> +	/* Make sure there's enough room for any header, and make it writable */
Please rephrase: /* Make sure there is enough writable headroom */

Just do:
	ret = skb_cow_head(skb, drvr->hdrlen);
	if (ret < 0) {
		brcmf_err("%s: skb_cow_head failed\n",
			  brcmf_ifname(ifp));
> +	if (skb_cow_head(skb, drvr->hdrlen)) {
>  		dev_kfree_skb(skb);
> -		skb = skb2;
> -		if (skb == NULL) {
> -			brcmf_err("%s: skb_realloc_headroom failed\n",
> -				  brcmf_ifname(ifp));
> -			ret = -ENOMEM;
> -			goto done;
> -		}
> +		ret = -ENOMEM;
	... and drop this assignment.

> +		goto done;
>  	}
>  
>  	/* validate length for ether packet */
> 

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 24-4-2017 15:03, James Hughes wrote:
> The driver was making changes to the skb_header without
> ensuring it was writable (i.e. uncloned).
> This patch also removes some boiler plate header size
> checking/adjustment code as that is also handled by the
> skb_cow_header function used to make header writable.
> 
> This patch depends on
> brcmfmac: Ensure pointer correctly set if skb data location changes
> 
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
> Changes in v2
>   Makes the _cow_ call at the entry point of the skb in to the
>   stack, means only needs to be done once, and error handling
>   is easier.
>   Split a separate minor bug fix off to a separate patch (which
>   this patch depends on)

Hi James,

Can you please indicate in this section that you want it applied for
4.12 as it is an important fix. Probably will have to wait until after
the merge window before it can get in the wireless-drivers repo.

Regards,
Arend

>  .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9b7c19a508ac..88f8675a94c2 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>  		goto done;
>  	}
>  
> -	/* Make sure there's enough room for any header */
> -	if (skb_headroom(skb) < drvr->hdrlen) {
> -		struct sk_buff *skb2;
> -
> -		brcmf_dbg(INFO, "%s: insufficient headroom\n",
> -			  brcmf_ifname(ifp));
> -		drvr->bus_if->tx_realloc++;
> -		skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
> +	/* Make sure there's enough room for any header, and make it writable */
> +	if (skb_cow_head(skb, drvr->hdrlen)) {
>  		dev_kfree_skb(skb);
> -		skb = skb2;
> -		if (skb == NULL) {
> -			brcmf_err("%s: skb_realloc_headroom failed\n",
> -				  brcmf_ifname(ifp));
> -			ret = -ENOMEM;
> -			goto done;
> -		}
> +		ret = -ENOMEM;
> +		goto done;
>  	}
>  
>  	/* validate length for ether packet */
> 

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Arend Van Spriel]

On 25-4-2017 9:38, Arend Van Spriel wrote:
> On 24-4-2017 15:03, James Hughes wrote:
>> The driver was making changes to the skb_header without
>> ensuring it was writable (i.e. uncloned).
>> This patch also removes some boiler plate header size
>> checking/adjustment code as that is also handled by the
>> skb_cow_header function used to make header writable.
>>
>> This patch depends on
>> brcmfmac: Ensure pointer correctly set if skb data location changes
>>
>> Signed-off-by: James Hughes <james.hughes-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>> ---
>> Changes in v2
>>   Makes the _cow_ call at the entry point of the skb in to the
>>   stack, means only needs to be done once, and error handling
>>   is easier.
>>   Split a separate minor bug fix off to a separate patch (which
>>   this patch depends on)
> 
> Hi James,
> 
> Can you please indicate in this section that you want it applied for
> 4.12 as it is an important fix. Probably will have to wait until after
> the merge window before it can get in the wireless-drivers repo.

Hi Kalle,

I should have checked kernel mailing list first as Linus added another
rc cycle. So can this patch still go in wireless-drivers-next for 4.12
kernel. I want it for stable as well, but I will look at that when it
lands upstream.

Regards,
Arend

> Regards,
> Arend
> 
>>  .../net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 19 ++++---------------
>>  1 file changed, 4 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index 9b7c19a508ac..88f8675a94c2 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>>  		goto done;
>>  	}
>>  
>> -	/* Make sure there's enough room for any header */
>> -	if (skb_headroom(skb) < drvr->hdrlen) {
>> -		struct sk_buff *skb2;
>> -
>> -		brcmf_dbg(INFO, "%s: insufficient headroom\n",
>> -			  brcmf_ifname(ifp));
>> -		drvr->bus_if->tx_realloc++;
>> -		skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
>> +	/* Make sure there's enough room for any header, and make it writable */
>> +	if (skb_cow_head(skb, drvr->hdrlen)) {
>>  		dev_kfree_skb(skb);
>> -		skb = skb2;
>> -		if (skb == NULL) {
>> -			brcmf_err("%s: skb_realloc_headroom failed\n",
>> -				  brcmf_ifname(ifp));
>> -			ret = -ENOMEM;
>> -			goto done;
>> -		}
>> +		ret = -ENOMEM;
>> +		goto done;
>>  	}
>>  
>>  	/* validate length for ether packet */
>>

Re: [PATCH v2] brcmfmac: Make skb header writable before use

[Kalle Valo]

Arend Van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> writes:

> On 25-4-2017 9:38, Arend Van Spriel wrote:
>> On 24-4-2017 15:03, James Hughes wrote:
>>> The driver was making changes to the skb_header without
>>> ensuring it was writable (i.e. uncloned).
>>> This patch also removes some boiler plate header size
>>> checking/adjustment code as that is also handled by the
>>> skb_cow_header function used to make header writable.
>>>
>>> This patch depends on
>>> brcmfmac: Ensure pointer correctly set if skb data location changes
>>>
>>> Signed-off-by: James Hughes <james.hughes-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>>> ---
>>> Changes in v2
>>>   Makes the _cow_ call at the entry point of the skb in to the
>>>   stack, means only needs to be done once, and error handling
>>>   is easier.
>>>   Split a separate minor bug fix off to a separate patch (which
>>>   this patch depends on)
>> 
>> Hi James,
>> 
>> Can you please indicate in this section that you want it applied for
>> 4.12 as it is an important fix. Probably will have to wait until after
>> the merge window before it can get in the wireless-drivers repo.
>
> Hi Kalle,
>
> I should have checked kernel mailing list first as Linus added another
> rc cycle. So can this patch still go in wireless-drivers-next for 4.12
> kernel.

Yup, I'll queue it for 4.12.

> I want it for stable as well, but I will look at that when it lands
> upstream.

Ok, so I don't add any stable tag during commit.

-- 
Kalle Valo