netdev - RE: [RFC PATCH v2 04/12] ixgbevf: Driver main and ethool interface module and main header

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]

Date:	Mon, 21 Dec 2009 10:18:22 -0800
From:	"Rose, Gregory V" <gregory.v.rose@...el.com>
To:	Simon Horman <horms@...ge.net.au>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>
Subject: RE: [RFC PATCH v2 04/12] ixgbevf: Driver main and ethool interface
 module and main header

-----Original Message-----
From: Simon Horman [mailto:horms@...ge.net.au] 
Sent: Monday, December 21, 2009 12:00 AM
To: Kirsher, Jeffrey T
Cc: netdev@...r.kernel.org; gospo@...hat.com; Rose, Gregory V
Subject: Re: [RFC PATCH v2 04/12] ixgbevf: Driver main and ethool interface module and main header

> +
> +	if (new_rx_count != adapter->rx_ring_count) {
> +		memcpy(rx_ring, adapter->rx_ring,
> +		       adapter->num_rx_queues * sizeof(struct ixgbevf_ring));
> +		for (i = 0; i < adapter->num_rx_queues; i++) {
> +			rx_ring[i].count = new_rx_count;
> +			err = ixgbevf_setup_rx_resources(adapter,
> +							 &rx_ring[i]);
> +			if (err) {
> +				while (i) {
> +					i--;
> +					ixgbevf_free_rx_resources(adapter,
> +								  &rx_ring[i]);
> +				}
> +				goto err_setup;

				Are some calls to ixgbevf_free_tx_resources()
				needed here? Perhaps the ones for
				on rx_ring failure could be moved down
				to an err_tx_clear or similar?
				(sorry for the shoddy name)

Let me have a look at it and see what I can (or should) do with it.

> +			}
> +			rx_ring[i].v_idx = adapter->rx_ring[i].v_idx;
> +		}
> +		need_update = true;
> +	}
> +
> +	/* if rings need to be updated, here's the place to do it in one shot */
> +	if (need_update) {
> +		if (netif_running(netdev))
> +			ixgbevf_down(adapter);
> +
> +		/* tx */
> +		if (new_tx_count != adapter->tx_ring_count) {
> +			kfree(adapter->tx_ring);
> +			adapter->tx_ring = tx_ring;
> +			tx_ring = NULL;
> +			adapter->tx_ring_count = new_tx_count;
> +		}
> +
> +		/* rx */
> +		if (new_rx_count != adapter->rx_ring_count) {
> +			kfree(adapter->rx_ring);
> +			adapter->rx_ring = rx_ring;
> +			rx_ring = NULL;
> +			adapter->rx_ring_count = new_rx_count;
> +		}
> +	}

	Can tx_ring and rx_ring leak here?

Maybe?  Let me look at it a bit closer and see.

I've also looked at your comments to 2/12 and will see what I can do to incorporate your suggestions there.  I appreciate the close review of the code.

- Greg

--
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