Re: [PATCHv3] staging: Check for Null allocated skb in fw_download_code
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly. In additon I removed the variable, rt_status as it's easier to read this function's return value with just true or false and rt status is a unneeded variable for the bool return of this function.
Signed-off-by: Nicholas Krause <xerofoify@gmail.com> --- drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c index 1a95d1f..66d83f8 100644 --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c @@ -36,7 +36,6 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address, u32 buffer_len) { struct r8192_priv *priv = rtllib_priv(dev); - bool rt_status = true; u16 frag_threshold; u16 frag_length, frag_offset = 0; int i; @@ -61,6 +60,8 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address, }
skb = dev_alloc_skb(frag_length + 4); + if (skb == NULL) + return false; 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; @@ -99,7 +100,7 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
write_nic_byte(dev, TPPoll, TPPoll_CQ);
- return rt_status; + return true; }
static bool CPUcheck_maincodeok_turnonCPU(struct net_device *dev) -- 1.9.1
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline. If someone wants to send it out for me and state it's from me that would be great. Nick
-----Original Message----- From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces@kernelnewbies.org] On Behalf Of Nick Krause Sent: Tuesday, August 12, 2014 1:20 PM To: kernelnewbies Subject: Re: [PATCHv3] staging: Check for Null allocated skb in fw_download_code
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly. In additon I removed the variable, rt_status as it's easier to read this function's return value with just true or false and rt status is a unneeded variable for the bool return of this function.
Signed-off-by: Nicholas Krause <xerofoify@gmail.com> --- drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c index 1a95d1f..66d83f8 100644 --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c @@ -36,7 +36,6 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address, u32 buffer_len) { struct r8192_priv *priv = rtllib_priv(dev); - bool rt_status = true; u16 frag_threshold; u16 frag_length, frag_offset = 0; int i; @@ -61,6 +60,8 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address, }
skb = dev_alloc_skb(frag_length + 4); + if (skb == NULL) + return false; 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; @@ -99,7 +100,7 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
write_nic_byte(dev, TPPoll, TPPoll_CQ);
- return rt_status; + return true; }
static bool CPUcheck_maincodeok_turnonCPU(struct net_device *dev) -- 1.9.1
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline. If someone wants to send it out for me and state it's from me that would be great. Nick
While the avoidance of dereferencing NULL pointers in the kernel is a laudable goal, what will be the effect of the call to write_nic_byte() at the end of the function not happening should the call to dev_alloc_skb() return NULL? Jeff Haran
On Tue, 12 Aug 2014 20:46:56 -0000, Jeff Haran said:
While the avoidance of dereferencing NULL pointers in the kernel is a laudable goal, what will be the effect of the call to write_nic_byte() at the end of the function not happening should the call to dev_alloc_skb() return NULL?
Damn. I missed that one, good catch.
On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly.
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline.
No, you're having trouble getting this into mainline because you are *STILL* persisting in submitting patches that are buggy. In this case, the problem is you *DON'T* exit the function cleanly. Note your patch causes an immediate return from inside a do/while loop, which *also* contains: skb_put(skb, i); So if there's (say) 3 fragments needed, and we fail on the allocation of the third one, you just leaked references to the first two fragments, and never actually clean up the allocations, so we have references to leaked memory. And leaking memory in a case where we're almost certainly very close to OOM isn't exactly a good idea. Yes, failing to check the return code is a bug - but so is failing to unwind the allocations already made. It took me all of a minute to spot this issue - the only clue needed was that there was a '*_put()' call in the function, which should be a warning flag that reference counting needs to be checked. Greg: Consider this a NACK of this patch. Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*. Seriously Nick. *PLEASE* stop posting patches until you've gotten a better handle on what code maintenance really entails.
On Tue Aug 12 14, Valdis.Kletnieks@vt.edu wrote:
On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly.
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline.
No, you're having trouble getting this into mainline because you are *STILL* persisting in submitting patches that are buggy.
In this case, the problem is you *DON'T* exit the function cleanly.
Note your patch causes an immediate return from inside a do/while loop, which *also* contains:
skb_put(skb, i);
So if there's (say) 3 fragments needed, and we fail on the allocation of the third one, you just leaked references to the first two fragments, and never actually clean up the allocations, so we have references to leaked memory. And leaking memory in a case where we're almost certainly very close to OOM isn't exactly a good idea. Yes, failing to check the return code is a bug - but so is failing to unwind the allocations already made.
It took me all of a minute to spot this issue - the only clue needed was that there was a '*_put()' call in the function, which should be a warning flag that reference counting needs to be checked.
Greg: Consider this a NACK of this patch.
Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
Seriously Nick. *PLEASE* stop posting patches until you've gotten a better handle on what code maintenance really entails.
A minor point, but I don't believe skb_put() has anything to do with reference counting, though the name would make you think so. sk_buff reference counting happens in skb_get() and the *free_skb() routines from the looks of it. Jerry
On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
On Tue Aug 12 14, Valdis.Kletnieks@vt.edu wrote:
On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly.
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline.
No, you're having trouble getting this into mainline because you are *STILL* persisting in submitting patches that are buggy.
In this case, the problem is you *DON'T* exit the function cleanly.
Note your patch causes an immediate return from inside a do/while loop, which *also* contains:
skb_put(skb, i);
So if there's (say) 3 fragments needed, and we fail on the allocation of the third one, you just leaked references to the first two fragments, and never actually clean up the allocations, so we have references to leaked memory. And leaking memory in a case where we're almost certainly very close to OOM isn't exactly a good idea. Yes, failing to check the return code is a bug - but so is failing to unwind the allocations already made.
It took me all of a minute to spot this issue - the only clue needed was that there was a '*_put()' call in the function, which should be a warning flag that reference counting needs to be checked.
Greg: Consider this a NACK of this patch.
Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
Seriously Nick. *PLEASE* stop posting patches until you've gotten a better handle on what code maintenance really entails.
A minor point, but I don't believe skb_put() has anything to do with reference counting, though the name would make you think so. sk_buff reference counting happens in skb_get() and the *free_skb() routines from the looks of it.
Jerry
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies Sorry Guys, I will reread the function and send out a patch that is bug free and actually works. Thanks Greg for at least reading it for now :). Cheers Nick
On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
On Tue Aug 12 14, Valdis.Kletnieks@vt.edu wrote:
On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly.
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline.
No, you're having trouble getting this into mainline because you are *STILL* persisting in submitting patches that are buggy.
In this case, the problem is you *DON'T* exit the function cleanly.
Note your patch causes an immediate return from inside a do/while loop, which *also* contains:
skb_put(skb, i);
So if there's (say) 3 fragments needed, and we fail on the allocation of the third one, you just leaked references to the first two fragments, and never actually clean up the allocations, so we have references to leaked memory. And leaking memory in a case where we're almost certainly very close to OOM isn't exactly a good idea. Yes, failing to check the return code is a bug - but so is failing to unwind the allocations already made.
It took me all of a minute to spot this issue - the only clue needed was that there was a '*_put()' call in the function, which should be a warning flag that reference counting needs to be checked.
Greg: Consider this a NACK of this patch.
Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
Seriously Nick. *PLEASE* stop posting patches until you've gotten a better handle on what code maintenance really entails.
A minor point, but I don't believe skb_put() has anything to do with reference counting, though the name would make you think so. sk_buff reference counting happens in skb_get() and the *free_skb() routines from the looks of it.
Jerry
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies Sorry Guys, I will reread the function and send out a patch that is bug free and actually works. Thanks Greg for at least reading it for now :). Cheers Nick
I looked into what Jerry states and he seems to be right. I will paste the code of skb_put for your convenience to check if Jerry and me are right. In addition about the call to write_nic_byte(dev, TPPoll, TPPoll_CQ); is there any good place to put it besides the end of the function seems there isn't. I was wondering if I rewrote this to break out of the loop and keep everything else the same is Ok. Nick
On Tue, Aug 12, 2014 at 10:41 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
On Tue Aug 12 14, Valdis.Kletnieks@vt.edu wrote:
On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. This entry states that we are not checking the skb allocated in fw_download_code for NULL and after checking it ,I fixed it to check for the NULL value before returning false and exiting fw_download_code cleanly.
I am trying to get this patch merged and after my issues with the kernel community, I can't get this into the mainline.
No, you're having trouble getting this into mainline because you are *STILL* persisting in submitting patches that are buggy.
In this case, the problem is you *DON'T* exit the function cleanly.
Note your patch causes an immediate return from inside a do/while loop, which *also* contains:
skb_put(skb, i);
So if there's (say) 3 fragments needed, and we fail on the allocation of the third one, you just leaked references to the first two fragments, and never actually clean up the allocations, so we have references to leaked memory. And leaking memory in a case where we're almost certainly very close to OOM isn't exactly a good idea. Yes, failing to check the return code is a bug - but so is failing to unwind the allocations already made.
It took me all of a minute to spot this issue - the only clue needed was that there was a '*_put()' call in the function, which should be a warning flag that reference counting needs to be checked.
Greg: Consider this a NACK of this patch.
Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
Seriously Nick. *PLEASE* stop posting patches until you've gotten a better handle on what code maintenance really entails.
A minor point, but I don't believe skb_put() has anything to do with reference counting, though the name would make you think so. sk_buff reference counting happens in skb_get() and the *free_skb() routines from the looks of it.
Jerry
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies Sorry Guys, I will reread the function and send out a patch that is bug free and actually works. Thanks Greg for at least reading it for now :). Cheers Nick
I looked into what Jerry states and he seems to be right. I will paste the code of skb_put for your convenience to check if Jerry and me are right. In addition about the call to write_nic_byte(dev, TPPoll, TPPoll_CQ); is there any good place to put it besides the end of the function seems there isn't. I was wondering if I rewrote this to break out of the loop and keep everything else the same is Ok.
Nick Sorry about the multiple emails. Here is skb_put for you all to read. Nick
1271 unsigned char *skb_put(struct sk_buff *skb, unsigned int len) 1272 { 1273 unsigned char *tmp = skb_tail_pointer(skb); 1274 SKB_LINEAR_ASSERT(skb); 1275 skb->tail += len; 1276 skb->len += len; 1277 if (unlikely(skb->tail > skb->end)) 1278 skb_over_panic(skb, len, __builtin_return_address(0)); 1279 return tmp; 1280 } 1281 EXPORT_SYMBOL(skb_put);
participants (4)
-
Jeff Haran -
Jerry Snitselaar -
Nick Krause -
Valdis.Kletnieks＠vt.edu