[PATCH v4 0/3] Staging: rtl8192e: Safer allocation and cleaner error handling

[PATCH v4 0/3] Staging: rtl8192e: Safer allocation and cleaner error handling

[Felix Schlepper]

v2:
  - replaced kmalloc with kzalloc against memory initialization defects,
    thus also removing a memset
  - made error handling more consistent

v3:
  - Split into smaller commits so that it's easier to review

v4:
  - clearer commit messages
  - added missing kfree

Felix Schlepper (3):
  Staging: rtl8192e: Use struct_size
  Staging: rtl8192e: Using kzalloc and delete memset
  Staging: rtl8192e: Cleaning up error handling

 drivers/staging/rtl8192e/rtllib_tx.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

--
2.36.1

[PATCH v4 1/3] Staging: rtl8192e: Use struct_size

[Felix Schlepper]

Using struct_size is encouraged because it is safer
than using sizeof and calculations by hand.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index 37715afb0210..f2ef32e943ae 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -205,8 +205,7 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
 	struct rtllib_txb *txb;
 	int i;
 
-	txb = kmalloc(sizeof(struct rtllib_txb) + (sizeof(u8 *) * nr_frags),
-		      gfp_mask);
+	txb = kmalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
 	if (!txb)
 		return NULL;
 
-- 
2.36.1

[PATCH v4 2/3] Staging: rtl8192e: Using kzalloc and delete memset

[Felix Schlepper]

By using kzalloc, we can delete a memset.  The practical difference
is that using kzalloc() will zero out the txb->fragments[] array.
The original code worked fine, but zeroing everything seems nicer.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index f2ef32e943ae..1307cf55741a 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -205,11 +205,10 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
 	struct rtllib_txb *txb;
 	int i;
 
-	txb = kmalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
+	txb = kzalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
 	if (!txb)
 		return NULL;
 
-	memset(txb, 0, sizeof(struct rtllib_txb));
 	txb->nr_frags = nr_frags;
 	txb->frag_size = cpu_to_le16(txb_size);
 
-- 
2.36.1

[PATCH v4 3/3] Staging: rtl8192e: Cleaning up error handling

[Felix Schlepper]

Moved error handling to one common block.
This removes double checking if all txb->fragments[]
were initialized.
The original code worked fine, but this is cleaner.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_tx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index 1307cf55741a..42f81b23a144 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -214,19 +214,19 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
 
 	for (i = 0; i < nr_frags; i++) {
 		txb->fragments[i] = dev_alloc_skb(txb_size);
-		if (unlikely(!txb->fragments[i])) {
-			i--;
-			break;
-		}
+		if (unlikely(!txb->fragments[i]))
+			goto err_free;
 		memset(txb->fragments[i]->cb, 0, sizeof(txb->fragments[i]->cb));
 	}
-	if (unlikely(i != nr_frags)) {
-		while (i >= 0)
-			dev_kfree_skb_any(txb->fragments[i--]);
-		kfree(txb);
-		return NULL;
-	}
+
 	return txb;
+
+err_free:
+	while (--i >= 0)
+		dev_kfree_skb_any(txb->fragments[i]);
+	kfree(txb);
+
+	return NULL;
 }
 
 static int rtllib_classify(struct sk_buff *skb, u8 bIsAmsdu)
-- 
2.36.1

Re: [PATCH v4 0/3] Staging: rtl8192e: Safer allocation and cleaner error handling

[Dan Carpenter]

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH v4 0/3] Staging: rtl8192e: Safer allocation and cleaner error handling

[Greg KH]

On Thu, Jun 23, 2022 at 12:05:54PM +0200, Felix Schlepper wrote:
> 
> v2:
>   - replaced kmalloc with kzalloc against memory initialization defects,
>     thus also removing a memset
>   - made error handling more consistent
> 
> v3:
>   - Split into smaller commits so that it's easier to review
> 
> v4:
>   - clearer commit messages
>   - added missing kfree
> 
> Felix Schlepper (3):
>   Staging: rtl8192e: Use struct_size
>   Staging: rtl8192e: Using kzalloc and delete memset
>   Staging: rtl8192e: Cleaning up error handling
> 
>  drivers/staging/rtl8192e/rtllib_tx.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> --
> 2.36.1
> 

Why is this not properly threaded with the patches itself?  Please
resend so that our tools will pick them all up correctly.

thanks,

greg k-h

Re: [PATCH v4 0/3] Staging: rtl8192e: Safer allocation and cleaner error handling

[Felix Schlepper]

On 23.06.2022 12:47, Greg KH wrote:
> On Thu, Jun 23, 2022 at 12:05:54PM +0200, Felix Schlepper wrote:
> >
> > v2:
> >   - replaced kmalloc with kzalloc against memory initialization defects,
> >     thus also removing a memset
> >   - made error handling more consistent
> >
> > v3:
> >   - Split into smaller commits so that it's easier to review
> >
> > v4:
> >   - clearer commit messages
> >   - added missing kfree
> >
> > Felix Schlepper (3):
> >   Staging: rtl8192e: Use struct_size
> >   Staging: rtl8192e: Using kzalloc and delete memset
> >   Staging: rtl8192e: Cleaning up error handling
> >
> >  drivers/staging/rtl8192e/rtllib_tx.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > --
> > 2.36.1
> >
>
> Why is this not properly threaded with the patches itself?  Please
> resend so that our tools will pick them all up correctly.
>
> thanks,
>
> greg k-h
I am trying... I thought I figured it out with the help of kernelnewbies
irc, but I resend and it is still not threaded properly.
I tried to reply to the message-id with the patchseries,which worked
locally, but I am just
causing a mess on the mailing list at this point. Will try to figure
this out properly.

Cheers
Felix Schlepper

[PATCH v4 0/3] Staging: rtl8192e: Safer allocation and cleaner error handling

[Felix Schlepper]

v2:
  - replaced kmalloc with kzalloc against memory initialization defects,
    thus also removing a memset
  - made error handling more consistent

v3:
  - Split into smaller commits so that it's easier to review

v4:
  - clearer commit messages
  - added missing kfree

Felix Schlepper (3):
  Staging: rtl8192e: Use struct_size
  Staging: rtl8192e: Using kzalloc and delete memset
  Staging: rtl8192e: Cleaning up error handling

 drivers/staging/rtl8192e/rtllib_tx.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

--
2.36.1

[PATCH v4 1/3] Staging: rtl8192e: Use struct_size

[Felix Schlepper]

Using struct_size is encouraged because it is safer
than using sizeof and calculations by hand.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index 37715afb0210..f2ef32e943ae 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -205,8 +205,7 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
 	struct rtllib_txb *txb;
 	int i;
 
-	txb = kmalloc(sizeof(struct rtllib_txb) + (sizeof(u8 *) * nr_frags),
-		      gfp_mask);
+	txb = kmalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
 	if (!txb)
 		return NULL;
 
-- 
2.36.1

[PATCH v4 2/3] Staging: rtl8192e: Using kzalloc and delete memset

[Felix Schlepper]

By using kzalloc, we can delete a memset.  The practical difference
is that using kzalloc() will zero out the txb->fragments[] array.
The original code worked fine, but zeroing everything seems nicer.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index f2ef32e943ae..1307cf55741a 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -205,11 +205,10 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
 	struct rtllib_txb *txb;
 	int i;
 
-	txb = kmalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
+	txb = kzalloc(struct_size(txb, fragments, nr_frags), gfp_mask);
 	if (!txb)
 		return NULL;
 
-	memset(txb, 0, sizeof(struct rtllib_txb));
 	txb->nr_frags = nr_frags;
 	txb->frag_size = cpu_to_le16(txb_size);
 
-- 
2.36.1

[PATCH v4 3/3] Staging: rtl8192e: Cleaning up error handling

[Felix Schlepper]

Moved error handling to one common block.
This removes double checking if all txb->fragments[]
were initialized.
The original code worked fine, but this is cleaner.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_tx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
index 1307cf55741a..42f81b23a144 100644
--- a/drivers/staging/rtl8192e/rtllib_tx.c
+++ b/drivers/staging/rtl8192e/rtllib_tx.c
@@ -214,19 +214,19 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
 
 	for (i = 0; i < nr_frags; i++) {
 		txb->fragments[i] = dev_alloc_skb(txb_size);
-		if (unlikely(!txb->fragments[i])) {
-			i--;
-			break;
-		}
+		if (unlikely(!txb->fragments[i]))
+			goto err_free;
 		memset(txb->fragments[i]->cb, 0, sizeof(txb->fragments[i]->cb));
 	}
-	if (unlikely(i != nr_frags)) {
-		while (i >= 0)
-			dev_kfree_skb_any(txb->fragments[i--]);
-		kfree(txb);
-		return NULL;
-	}
+
 	return txb;
+
+err_free:
+	while (--i >= 0)
+		dev_kfree_skb_any(txb->fragments[i]);
+	kfree(txb);
+
+	return NULL;
 }
 
 static int rtllib_classify(struct sk_buff *skb, u8 bIsAmsdu)
-- 
2.36.1

Re: [PATCH v4 3/3] Staging: rtl8192e: Cleaning up error handling

[Andy Shevchenko]

On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <f3sch.git@outlook.com> wrote:
>
> Moved error handling to one common block.
> This removes double checking if all txb->fragments[]
> were initialized.
> The original code worked fine, but this is cleaner.

...

> +err_free:
> +       while (--i >= 0)

  while (i--)

will suffice.

> +               dev_kfree_skb_any(txb->fragments[i]);
> +       kfree(txb);
> +
> +       return NULL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] Staging: rtl8192e: Cleaning up error handling

[Dan Carpenter]

On Thu, Jun 23, 2022 at 11:20:49PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <f3sch.git@outlook.com> wrote:
> >
> > Moved error handling to one common block.
> > This removes double checking if all txb->fragments[]
> > were initialized.
> > The original code worked fine, but this is cleaner.
> 
> ...
> 
> > +err_free:
> > +       while (--i >= 0)
> 
>   while (i--)
> 
> will suffice.
> 

Either one is fine.  You prefer this format.  I prefer that other
format.  I told Felix he could use either format without expressing any
bias and he chose my format.  That means he loves me more.

regards,
dan carpenter

Re: [PATCH v4 3/3] Staging: rtl8192e: Cleaning up error handling

[Felix Schlepper]

On 24.06.2022 08:21, Dan Carpenter wrote:
> On Thu, Jun 23, 2022 at 11:20:49PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <f3sch.git@outlook.com> wrote:
> > >
> > > Moved error handling to one common block.
> > > This removes double checking if all txb->fragments[]
> > > were initialized.
> > > The original code worked fine, but this is cleaner.
> >
> > ...
> >
> > > +err_free:
> > > +       while (--i >= 0)
> >
> >   while (i--)
> >
> > will suffice.
> >
>
> Either one is fine.  You prefer this format.  I prefer that other
> format.  I told Felix he could use either format without expressing any
> bias and he chose my format.  That means he loves me more.
>
> regards,
> dan carpenter
Andy's advice is of course much appreciated but I prefer Dan's style <3.

On another note, the thread is quite messy now, since all my previous
failed attempts to send are now threaded to this one.
How would I go about solving this? Sending another v5 version, the v4
with [RESEND PATCH v4...] or does this even need any action on my part?

Cheers
Felix Schlepper

Re: [PATCH v4 3/3] Staging: rtl8192e: Cleaning up error handling

[Greg Kroah-Hartman]

On Fri, Jun 24, 2022 at 10:41:42AM +0200, Felix Schlepper wrote:
> On 24.06.2022 08:21, Dan Carpenter wrote:
> > On Thu, Jun 23, 2022 at 11:20:49PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 23, 2022 at 10:44 PM Felix Schlepper <f3sch.git@outlook.com> wrote:
> > > >
> > > > Moved error handling to one common block.
> > > > This removes double checking if all txb->fragments[]
> > > > were initialized.
> > > > The original code worked fine, but this is cleaner.
> > >
> > > ...
> > >
> > > > +err_free:
> > > > +       while (--i >= 0)
> > >
> > >   while (i--)
> > >
> > > will suffice.
> > >
> >
> > Either one is fine.  You prefer this format.  I prefer that other
> > format.  I told Felix he could use either format without expressing any
> > bias and he chose my format.  That means he loves me more.
> >
> > regards,
> > dan carpenter
> Andy's advice is of course much appreciated but I prefer Dan's style <3.
> 
> On another note, the thread is quite messy now, since all my previous
> failed attempts to send are now threaded to this one.
> How would I go about solving this? Sending another v5 version, the v4
> with [RESEND PATCH v4...] or does this even need any action on my part?

Please resend a v5, as I now have 3 different copies of a v4 series in
my inbox, which makes no sense at all.

Remember, make it obvious as to what to do for those of us who have to
handle 1000+ emails a day...

thanks,

greg k-h