Staging: rtl8192e: Safer allocation and cleaner error handling

PATCH Superseded Felix Schlepper 4 participants 11 messages Jun 23, 2022 – Jun 24, 2022 Staging

lore

Share & tools

lore

Felix Schlepper

author

2022-06-23 10:06 UTC #1 lore raw

PATCH v4 1/3 Staging: rtl8192e: Use struct_size

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

Signed-off-by Felix Schlepper <[email protected]>

---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/rtl8192e/rtllib_tx.c +1 -2

index 37715afb0210..f2ef32e943ae 100644
@@ -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;

Signature · 1 line

2.36.1

Felix Schlepper

author

2022-06-23 20:29 UTC #2 lore raw

PATCH v4 1/3 Staging: rtl8192e: Use struct_size

In reply to Felix Schlepper

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

Signed-off-by Felix Schlepper <[email protected]>

---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/rtl8192e/rtllib_tx.c +1 -2

index 37715afb0210..f2ef32e943ae 100644
@@ -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;

Signature · 1 line

2.36.1

Felix Schlepper

author

2022-06-23 10:06 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/rtl8192e/rtllib_tx.c +1 -2

index f2ef32e943ae..1307cf55741a 100644
@@ -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);

Signature · 1 line

2.36.1

Felix Schlepper

author

2022-06-23 20:29 UTC #4 lore raw

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

In reply to Felix Schlepper

Signed-off-by Felix Schlepper <[email protected]>

---
 drivers/staging/rtl8192e/rtllib_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/rtl8192e/rtllib_tx.c +1 -2

index f2ef32e943ae..1307cf55741a 100644
@@ -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);

Signature · 1 line

2.36.1

Felix Schlepper

author

2022-06-23 10:06 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/rtl8192e/rtllib_tx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/staging/rtl8192e/rtllib_tx.c +10 ~~-10~~

index 1307cf55741a..42f81b23a144 100644
@@ -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)

Signature · 1 line

2.36.1

Felix Schlepper

author

2022-06-23 20:29 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/rtl8192e/rtllib_tx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/staging/rtl8192e/rtllib_tx.c +10 ~~-10~~

index 1307cf55741a..42f81b23a144 100644
@@ -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)

Signature · 1 line

2.36.1

Andy Shevchenko

2022-06-23 21:21 UTC #7 lore raw

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

Quoted 5 lines · 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.

...

drivers/staging/rtl8192e/rtllib_tx.c static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size, · line 225

Quoted 2 lines

drivers/staging/rtl8192e/rtllib_tx.c +2 -0
+err_free:
+       while (--i >= 0)

while (i--)

will suffice.

Quoted 4 lines

drivers/staging/rtl8192e/rtllib_tx.c +4 -0

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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dan Carpenter

2022-06-24 05:21 UTC #8 lore raw

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

Quoted 7 lines · Andy Shevchenko

Quoted 5 lines · 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.

Quoted 9 lines

...
drivers/staging/rtl8192e/rtllib_tx.c static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size, · line 225
Quoted 2 lines
drivers/staging/rtl8192e/rtllib_tx.c +2 -0
+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

Felix Schlepper

author

2022-06-24 08:41 UTC #9 lore raw

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

Quoted 18 lines · Dan Carpenter

Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · 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.
Quoted 9 lines
...
drivers/staging/rtl8192e/rtllib_tx.c static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size, · line 225
Quoted 2 lines
drivers/staging/rtl8192e/rtllib_tx.c +2 -0
+err_free:
+       while (--i >= 0)
while (i--)
will suffice.

Quoted 6 lines

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

Greg Kroah-Hartman

maintainer

2022-06-24 11:44 UTC #10 lore raw

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

Quoted 23 lines · Felix Schlepper

Quoted 22 lines · Dan Carpenter
Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · 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.
Quoted 9 lines
...
drivers/staging/rtl8192e/rtllib_tx.c static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size, · line 225
Quoted 2 lines
drivers/staging/rtl8192e/rtllib_tx.c +2 -0
+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.

Quoted 8 lines

Quoted 2 lines
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