netdev - Re: [PATCH]: Third (final?) release of Sun Neptune driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20071005.160700.38700247.davem@davemloft.net>
Date:	Fri, 05 Oct 2007 16:07:00 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	shemminger@...ux-foundation.org
Cc:	netdev@...r.kernel.org, Ariel.Hendel@....com, greg.onufer@....com,
	jeff@...zik.org, Ashley.Saulsbury@....com, Matheos.Worku@....com
Subject: Re: [PATCH]: Third (final?) release of Sun Neptune driver

From: Stephen Hemminger <shemminger@...ux-foundation.org>
Date: Fri, 5 Oct 2007 08:35:56 -0700

> > +		BUG_ON(!page);
> 
> BUG_ON checks for NULL are useless if the code following is
> just going to dereference the pointer anyway. 

Agreed, all such cases fixed in the patch below, thanks!

> > +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> > +		tb = &rp->tx_buffs[idx];
> > +		BUG_ON(tb->skb != NULL);
> 
> > +		np->ops->unmap_page(np->device, tb->mapping,
> > +				    skb_shinfo(skb)->frags[i].size,
> > +				    DMA_TO_DEVICE);
> 
> Shouldn't this be skb_shinfo(tb->skb)->frags...

Nope, look at the logic, the assertion is making surethat
tb->skb == NULL.

When transmitting, the first descriptor holds the SKB pointer
and the rest of the descriptors for that packet explicitly
set the slots' SKB to NULL.

> > +		idx = NEXT_TX(rp, idx);
> > +	}
> > +
> > +	dev_kfree_skb(skb);
> 
> Should be dev_kfree_skb_any() if you plan on supporting
> netconsole.

I have no such plans at this time :-)

I think I've covered all of your feedback from this email,
thanks again for the review.

>From 418d57645f3132241eb180f9d381ef6541745e23 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@...set.davemloft.net>
Date: Fri, 5 Oct 2007 16:04:16 -0700
Subject: [PATCH] [NIU]: Remove BUG_ON() NULL pointer checks.

As mentioned by Stephen Hemminger, these are superfluous
if we are about to dereference the thing anyways.  The
OOPS has the same effect and will show the problem just
as clearly.

Signed-off-by: David S. Miller <davem@...emloft.net>
---
 drivers/net/niu.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 450d95e..c29235e 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -2134,7 +2134,6 @@ static int niu_rx_pkt_ignore(struct niu *np, struct rx_ring_info *rp)
 		addr = (val & RCR_ENTRY_PKT_BUF_ADDR) <<
 			RCR_ENTRY_PKT_BUF_ADDR_SHIFT;
 		page = niu_find_rxpage(rp, addr, &link);
-		BUG_ON(!page);
 
 		rcr_size = rp->rbr_sizes[(val & RCR_ENTRY_PKTBUFSZ) >>
 					 RCR_ENTRY_PKTBUFSZ_SHIFT];
@@ -2185,7 +2184,6 @@ static int niu_process_rx_pkt(struct niu *np, struct rx_ring_info *rp)
 		addr = (val & RCR_ENTRY_PKT_BUF_ADDR) <<
 			RCR_ENTRY_PKT_BUF_ADDR_SHIFT;
 		page = niu_find_rxpage(rp, addr, &link);
-		BUG_ON(!page);
 
 		rcr_size = rp->rbr_sizes[(val & RCR_ENTRY_PKTBUFSZ) >>
 					 RCR_ENTRY_PKTBUFSZ_SHIFT];
@@ -2294,8 +2292,6 @@ static int release_tx_packet(struct niu *np, struct tx_ring_info *rp, int idx)
 	u64 tx_flags;
 	int i, len;
 
-	BUG_ON(!skb);
-
 	tp = (struct tx_pkt_hdr *) skb->data;
 	tx_flags = le64_to_cpup(&tp->flags);
 
-- 
1.5.3.3

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html