[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nicholas Krause]

This patch checks if we are getting a Null allocated skb in the while/do
loop of this function. Further more no reference checking is needing
as skb_put is only working with the packet defined by skb and not allocating
a new one. Further more I am breaking out of the loop in order to make sure
we call nic_write_byte and cause no errors with writing the nic byte(s) of
private date and then call rt_status which I changed to false in the loop
with my additon of checking the allocated skb.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
index 1a95d1f..829af66 100644
--- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
+++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
@@ -61,6 +61,10 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
 		}
 
 		skb  = dev_alloc_skb(frag_length + 4);
+		if( skb == NULL) {
+			rt_status =  false ;
+			break;
+		}	
 		memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
 		tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
 		tcb_desc->queue_index = TXCMD_QUEUE;
-- 
1.9.1

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 11:12 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> This patch checks if we are getting a Null allocated skb in the while/do
> loop of this function. Further more no reference checking is needing
> as skb_put is only working with the packet defined by skb and not allocating
> a new one. Further more I am breaking out of the loop in order to make sure
> we call nic_write_byte and cause no errors with writing the nic byte(s) of
> private date and then call rt_status which I changed to false in the loop
> with my additon of checking the allocated skb.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> index 1a95d1f..829af66 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> @@ -61,6 +61,10 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
>                 }
>
>                 skb  = dev_alloc_skb(frag_length + 4);
> +               if( skb == NULL) {
> +                       rt_status =  false ;
> +                       break;
> +               }
>                 memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
>                 tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
>                 tcb_desc->queue_index = TXCMD_QUEUE;
> --
> 1.9.1
>
If you didn't read my other emails, skb_put is not allocating another
skb and therefore no memory leaks
as you guys stated. By the way thanks for the input, I should have
read the code more carefully first.
Nick

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nicholas Krause]

This patch checks if we are getting a Null allocated skb in the while/do
loop of this function. Further more no reference checking is needing
as skb_put is only working with the packet defined by skb and not allocating
a new one. Further more I am breaking out of the loop in order to make sure
we call nic_write_byte and cause no errors with writing the nic byte(s) of
private date and then call rt_status which I changed to false in the loop
with my additon of checking the allocated skb.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
index 1a95d1f..829af66 100644
--- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
+++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
@@ -61,6 +61,10 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
 		}
 
 		skb  = dev_alloc_skb(frag_length + 4);
+		if (skb == NULL) {
+			rt_status =  false;
+			break;
+		}
 		memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
 		tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
 		tcb_desc->queue_index = TXCMD_QUEUE;
-- 
1.9.1

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 11:24 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> This patch checks if we are getting a Null allocated skb in the while/do
> loop of this function. Further more no reference checking is needing
> as skb_put is only working with the packet defined by skb and not allocating
> a new one. Further more I am breaking out of the loop in order to make sure
> we call nic_write_byte and cause no errors with writing the nic byte(s) of
> private date and then call rt_status which I changed to false in the loop
> with my additon of checking the allocated skb.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> index 1a95d1f..829af66 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> @@ -61,6 +61,10 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
>                 }
>
>                 skb  = dev_alloc_skb(frag_length + 4);
> +               if (skb == NULL) {
> +                       rt_status =  false;
> +                       break;
> +               }
>                 memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
>                 tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
>                 tcb_desc->queue_index = TXCMD_QUEUE;
> --
> 1.9.1
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Sorry about the two patches please read only the second one, I send twice.
Nick

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Tue, 12 Aug 2014 23:24:32 -0400, Nicholas Krause said:
> This patch checks if we are getting a Null allocated skb in the while/do
> loop of this function.
>  		skb  = dev_alloc_skb(frag_length + 4);
> +		if (skb == NULL) {
> +			rt_status =  false;
> +			break;
> +		}

Nick, it's *STILL* wrong.  And although I admit I blew it on the refcount
issue, that doesn't change the fact that you're still leaking memory here.

Work this through.  Hand simulate it.  Pretend that 3 fragments are
needed, and that the first two succeed but the third one fails.

Who frees the first two fragments before you return?

For bonus points - explain under what conditions this bug can *possibly*
be triggered on an actual system.  Consider in your reply both (a) when this
code is called and (b) what the system state *should* be at that point, and
what it has to be for this bug to trigger.  Given your answer to (a) and (b),
extrapolate to what the next few hundredths of a second likely hold for
this system even after we've fixed the bug you're mis-fixing here.

Once you've done that, you'll hopefully understand why you're trying to fix
a bug from 2009.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/5b0c1af4/attachment.bin 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Manish Katiyar]

On Tue, Aug 12, 2014 at 10:35 PM, <Valdis.Kletnieks@vt.edu> wrote:

> On Tue, 12 Aug 2014 23:24:32 -0400, Nicholas Krause said:
> > This patch checks if we are getting a Null allocated skb in the while/do
> > loop of this function.
> >               skb  = dev_alloc_skb(frag_length + 4);
> > +             if (skb == NULL) {
> > +                     rt_status =  false;
> > +                     break;
> > +             }
>
> Nick, it's *STILL* wrong.  And although I admit I blew it on the refcount
> issue, that doesn't change the fact that you're still leaking memory here.
>
> Work this through.  Hand simulate it.  Pretend that 3 fragments are
> needed, and that the first two succeed but the third one fails.
>
> Who frees the first two fragments before you return?
>
> For bonus points - explain under what conditions this bug can *possibly*
> be triggered on an actual system.  Consider in your reply both (a) when
> this
> code is called and (b) what the system state *should* be at that point, and
> what it has to be for this bug to trigger.  Given your answer to (a) and
> (b),
> extrapolate to what the next few hundredths of a second likely hold for
> this system even after we've fixed the bug you're mis-fixing here.
>
> Once you've done that, you'll hopefully understand why you're trying to fix
> a bug from 2009.
>
>
And it may also be a good idea to post the logs regarding how you tested
your patch and verified that the fix works as expected. As you are asking
someone else to send the patch on behalf of you, anyone who volunteers
would like to verify that the patch is indeed tested properly before
forwarding.

Thanks -
Manish




>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140812/e9da9a4e/attachment.html 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Tue, 12 Aug 2014 22:53:37 -0700, Manish Katiyar said:

> And it may also be a good idea to post the logs regarding how you tested
> your patch and verified that the fix works as expected. As you are asking

I want to see how he sets up an environment where he can *trigger* the issue
reliably. :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/c37064cd/attachment.bin 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Max Filippov]

On Wed, Aug 13, 2014 at 5:56 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Tue, 12 Aug 2014 22:53:37 -0700, Manish Katiyar said:
>
>> And it may also be a good idea to post the logs regarding how you tested
>> your patch and verified that the fix works as expected. As you are asking
>
> I want to see how he sets up an environment where he can *trigger* the issue
> reliably. :)

No need to trigger it, faking it would be enough, e.g.:

-               skb  = dev_alloc_skb(frag_length + 4);
+        {
+                static int i;
+                if (++i < 3)
+                        skb  = dev_alloc_skb(frag_length + 4);
+                else
+                        skb = NULL;
+        }
+        if (skb == NULL) {
+                rt_status =  false;
+                break;
+        }

-- 
Thanks.
-- Max

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:

> No need to trigger it, faking it would be enough, e.g.:

> +                if (++i < 3)
> +                        skb  = dev_alloc_skb(frag_length + 4);
> +                else
> +                        skb = NULL;

Don't bet on this triggering on a real system without some additional
scaffolding - take a look at what the function is doing, and ask yourself
how many times it will be called on the average system :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/645e13da/attachment.bin 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Wed, Aug 13, 2014 at 12:02 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
>
>> No need to trigger it, faking it would be enough, e.g.:
>
>> +                if (++i < 3)
>> +                        skb  = dev_alloc_skb(frag_length + 4);
>> +                else
>> +                        skb = NULL;
>
> Don't bet on this triggering on a real system without some additional
> scaffolding - take a look at what the function is doing, and ask yourself
> how many times it will be called on the average system :)
Seems to be called a lot. In addition I can only build test this as I
don't have the hardware.
Cheers Nick

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Wed, Aug 13, 2014 at 12:55 PM, Nick Krause <xerofoify@gmail.com> wrote:
> On Wed, Aug 13, 2014 at 12:02 PM,  <Valdis.Kletnieks@vt.edu> wrote:
>> On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
>>
>>> No need to trigger it, faking it would be enough, e.g.:
>>
>>> +                if (++i < 3)
>>> +                        skb  = dev_alloc_skb(frag_length + 4);
>>> +                else
>>> +                        skb = NULL;
>>
>> Don't bet on this triggering on a real system without some additional
>> scaffolding - take a look at what the function is doing, and ask yourself
>> how many times it will be called on the average system :)
> Seems to be called a lot. In addition I can only build test this as I
> don't have the hardware.
> Cheers Nick


I did test my patch by doing a kernel build and I get this error,
drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge?
[-Werror=implicit-function-declaration]
    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
I am wondering how do I fix this, I will attach my patch so I can fix
this out and send a proper patch :).
By the way thanks for the help guys :).
Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-staging-Check-for-Null-return-of-fw_download_code.patch
Type: text/x-patch
Size: 1380 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/2e6775dd/attachment.bin 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Philipp Muhoray]

Am 2014-08-13 20:03, schrieb Nick Krause:
> On Wed, Aug 13, 2014 at 12:55 PM, Nick Krause <xerofoify@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 12:02 PM,  <Valdis.Kletnieks@vt.edu> wrote:
>>> On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
>>>
>>>> No need to trigger it, faking it would be enough, e.g.:
>>>> +                if (++i < 3)
>>>> +                        skb  = dev_alloc_skb(frag_length + 4);
>>>> +                else
>>>> +                        skb = NULL;
>>> Don't bet on this triggering on a real system without some additional
>>> scaffolding - take a look at what the function is doing, and ask yourself
>>> how many times it will be called on the average system :)
>> Seems to be called a lot. In addition I can only build test this as I
>> don't have the hardware.
>> Cheers Nick
>
> I did test my patch by doing a kernel build and I get this error,
> drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
> implicit declaration of function 'skb_quene_purge'
> [-Werror=implicit-function-declaration]
>      skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
> I am wondering how do I fix this, I will attach my patch so I can fix
> this out and send a proper patch :).
> By the way thanks for the help guys :).
> Nick
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
This error indicates that you are using a function that is not yet known 
to the compiler. Check if there's a prototype or function definition 
BEFORE it's actual use.

br,
phil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/02b2ac73/attachment.html 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Max Filippov]

On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
> I did test my patch by doing a kernel build and I get this error,
> drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
> implicit declaration of function ?skb_quene_purge?
> [-Werror=implicit-function-declaration]
>     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
> I am wondering how do I fix this, I will attach my patch so I can fix
> this out and send a proper patch :).

Typo, skb_queue_purge.

-- 
Thanks.
-- Max

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Jerry Snitselaar]

On Wed Aug 13 14, Max Filippov wrote:
> On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
> > I did test my patch by doing a kernel build and I get this error,
> > drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
> > implicit declaration of function ?skb_quene_purge?
> > [-Werror=implicit-function-declaration]
> >     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
> > I am wondering how do I fix this, I will attach my patch so I can fix
> > this out and send a proper patch :).
> 
> Typo, skb_queue_purge.
> 

I imagine that array index isn't going to go over very well.

Jerry

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Wed, Aug 13, 2014 at 2:25 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
> On Wed Aug 13 14, Max Filippov wrote:
>> On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
>> > I did test my patch by doing a kernel build and I get this error,
>> > drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
>> > implicit declaration of function ?skb_quene_purge?
>> > [-Werror=implicit-function-declaration]
>> >     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
>> > I am wondering how do I fix this, I will attach my patch so I can fix
>> > this out and send a proper patch :).
>>
>> Typo, skb_queue_purge.
>>
>
> I imagine that array index isn't going to go over very well.
>
> Jerry
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

I known the patch is premature but a NULL pointer and in a device
driver needs to be patched no matter what.
If some one reading this has the hardware , it would be great if they
can test this :).
Cheers Nick

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Wed, Aug 13, 2014 at 2:27 PM, Nick Krause <xerofoify@gmail.com> wrote:
> On Wed, Aug 13, 2014 at 2:25 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
>> On Wed Aug 13 14, Max Filippov wrote:
>>> On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
>>> > I did test my patch by doing a kernel build and I get this error,
>>> > drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
>>> > implicit declaration of function ?skb_quene_purge?
>>> > [-Werror=implicit-function-declaration]
>>> >     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
>>> > I am wondering how do I fix this, I will attach my patch so I can fix
>>> > this out and send a proper patch :).
>>>
>>> Typo, skb_queue_purge.
>>>
>>
>> I imagine that array index isn't going to go over very well.
>>
>> Jerry
>>
>> _______________________________________________
>> Kernelnewbies mailing list
>> Kernelnewbies at kernelnewbies.org
>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
> I known the patch is premature but a NULL pointer and in a device
> driver needs to be patched no matter what.
> If some one reading this has the hardware , it would be great if they
> can test this :).
> Cheers Nick


Also just checked and the patch is fixed and builds at least from my testing.
Nick

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Jeff Haran]

> -----Original Message-----
> From: kernelnewbies-bounces at kernelnewbies.org [mailto:kernelnewbies-
> bounces at kernelnewbies.org] On Behalf Of Nick Krause
> Sent: Wednesday, August 13, 2014 11:03 AM
> To: Valdis Kletnieks
> Cc: Max Filippov; kernelnewbies; Manish Katiyar
> Subject: Re: [PATCH] staging: Check for Null return of allocated skb in
> fw_download_code
> 
> > Seems to be called a lot. In addition I can only build test this as I
> > don't have the hardware.
> > Cheers Nick

This code is a device driver. I may be an old fuddy-duddy in this regard, but I don't inflict my code on others until I've tested it.

If you don't have the hardware, you can't have tested it. I think your patch is thus premature.

If you are unable to obtain the hardware for testing, you might want to consider working on parts of the kernel that are hardware independent.

Jeff Haran

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Wed, 13 Aug 2014 14:03:08 -0400, Nick Krause said:

> I did test my patch by doing a kernel build and I get this error,
> drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
> implicit declaration of function ???skb_quene_purge???
> [-Werror=implicit-function-declaration]
>     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

Well, this is a C 101 problem.

> I am wondering how do I fix this,

You fix this by not doing any further kernel hacking until you've gotten
a handle on *VERY BASIC* C development concepts.

Also, you're going to have to justify why you're being a total
dumbass and coding skb_waitQ[tcb_desc->queue_index] when the *obvious*
code is skb_waitQ[TXCMD_QUEUE] - yes, an optimizing compiler will do
that substitution, but code clarity is important.

Argh.  I may have to break out my +5 Trout of Smacking....

On Wed, 13 Aug 2014 15:01:45 -0400, Nick Krause said:
> This is the fixed patch, I do get an error about uninitialized
> variables

Are you *trying* to get put in *everybody's* killfile?

> If someone wants to send this out, please do so as this is has been
> built and applied tested.

And you think that *anybody* wants to upstream a patch from you that even
*you* admit still has trouble??!?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/d83ebb86/attachment.bin 

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Wed, Aug 13, 2014 at 3:17 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Wed, 13 Aug 2014 14:03:08 -0400, Nick Krause said:
>
>> I did test my patch by doing a kernel build and I get this error,
>> drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
>> implicit declaration of function ?skb_quene_purge?
>> [-Werror=implicit-function-declaration]
>>     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
>
> Well, this is a C 101 problem.
>
>> I am wondering how do I fix this,
>
> You fix this by not doing any further kernel hacking until you've gotten
> a handle on *VERY BASIC* C development concepts.
>
> Also, you're going to have to justify why you're being a total
> dumbass and coding skb_waitQ[tcb_desc->queue_index] when the *obvious*
> code is skb_waitQ[TXCMD_QUEUE] - yes, an optimizing compiler will do
> that substitution, but code clarity is important.
>
> Argh.  I may have to break out my +5 Trout of Smacking....
>
> On Wed, 13 Aug 2014 15:01:45 -0400, Nick Krause said:
>> This is the fixed patch, I do get an error about uninitialized
>> variables
>
> Are you *trying* to get put in *everybody's* killfile?
>
>> If someone wants to send this out, please do so as this is has been
>> built and applied tested.
>
> And you think that *anybody* wants to upstream a patch from you that even
> *you* admit still has trouble??!?

I checked the patched and it worked , uninitialized variables are
common and I hit 12 of them.
Nick

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Nick Krause]

On Wed, Aug 13, 2014 at 3:24 PM, Nick Krause <xerofoify@gmail.com> wrote:
> On Wed, Aug 13, 2014 at 3:17 PM,  <Valdis.Kletnieks@vt.edu> wrote:
>> On Wed, 13 Aug 2014 14:03:08 -0400, Nick Krause said:
>>
>>> I did test my patch by doing a kernel build and I get this error,
>>> drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
>>> implicit declaration of function ?skb_quene_purge?
>>> [-Werror=implicit-function-declaration]
>>>     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);
>>
>> Well, this is a C 101 problem.
>>
>>> I am wondering how do I fix this,
>>
>> You fix this by not doing any further kernel hacking until you've gotten
>> a handle on *VERY BASIC* C development concepts.
>>
>> Also, you're going to have to justify why you're being a total
>> dumbass and coding skb_waitQ[tcb_desc->queue_index] when the *obvious*
>> code is skb_waitQ[TXCMD_QUEUE] - yes, an optimizing compiler will do
>> that substitution, but code clarity is important.
>>
>> Argh.  I may have to break out my +5 Trout of Smacking....
>>
>> On Wed, 13 Aug 2014 15:01:45 -0400, Nick Krause said:
>>> This is the fixed patch, I do get an error about uninitialized
>>> variables
>>
>> Are you *trying* to get put in *everybody's* killfile?
>>
>>> If someone wants to send this out, please do so as this is has been
>>> built and applied tested.
>>
>> And you think that *anybody* wants to upstream a patch from you that even
>> *you* admit still has trouble??!?
>
> I checked the patched and it worked , uninitialized variables are
> common and I hit 12 of them.
> Nick

Valdis,
Sorry about the stupid question , I assumed it was something else I was fixing.
In addition I fixed up the patch and there are not more errors based on using
gcc's optimization features.

Sorry Nick :(

[PATCH] staging: Check for Null return of allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Wed, 13 Aug 2014 12:55:00 -0400, Nick Krause said:
> On Wed, Aug 13, 2014 at 12:02 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> > On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
> >
> >> No need to trigger it, faking it would be enough, e.g.:
> >
> >> +                if (++i < 3)
> >> +                        skb  = dev_alloc_skb(frag_length + 4);
> >> +                else
> >> +                        skb = NULL;
> >
> > Don't bet on this triggering on a real system without some additional
> > scaffolding - take a look at what the function is doing, and ask yourself
> > how many times it will be called on the average system :)

> Seems to be called a lot.

Oh? Explain to us what this function is actually doing, in general terms,
and then give us a rough estimate of how many times per boot it's called.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140813/030850b6/attachment.bin