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:	Tue, 22 Dec 2009 16:05:59 -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

[snip]

>
>> +
>> +static int ixgbevf_set_ringparam(struct net_device *netdev,
>> +				 struct ethtool_ringparam *ring)
>> +{
>> +	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
>> +	struct ixgbevf_ring *tx_ring, *rx_ring;
>> +	int i, err;
>> +	u32 new_rx_count, new_tx_count;
>> +	bool need_update = false;
>> +
>> +	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>> +		return -EINVAL;
>> +
>> +	new_rx_count = max(ring->rx_pending, (u32)IXGBEVF_MIN_RXD);
>> +	new_rx_count = min(new_rx_count, (u32)IXGBEVF_MAX_RXD);
>> +	new_rx_count = ALIGN(new_rx_count,
>IXGBE_REQ_RX_DESCRIPTOR_MULTIPLE);
>> +
>> +	new_tx_count = max(ring->tx_pending, (u32)IXGBEVF_MIN_TXD);
>> +	new_tx_count = min(new_tx_count, (u32)IXGBEVF_MAX_TXD);
>> +	new_tx_count = ALIGN(new_tx_count,
>IXGBE_REQ_TX_DESCRIPTOR_MULTIPLE);
>> +
>> +	if ((new_tx_count == adapter->tx_ring->count) &&
>> +	    (new_rx_count == adapter->rx_ring->count)) {
>> +		/* nothing to do */
>> +		return 0;
>> +	}
>> +
>> +	while (test_and_set_bit(__IXGBEVF_RESETTING, &adapter->state))
>> +		msleep(1);
>> +
>> +	tx_ring = kcalloc(adapter->num_tx_queues,
>> +			       sizeof(struct ixgbevf_ring), GFP_KERNEL);
>> +	if (!tx_ring) {
>> +		err = -ENOMEM;
>> +		goto err_setup;
>> +	}
>> +
>> +	if (new_tx_count != adapter->tx_ring_count) {
>> +		memcpy(tx_ring, adapter->tx_ring,
>> +		       adapter->num_tx_queues * sizeof(struct
>ixgbevf_ring));
>> +		for (i = 0; i < adapter->num_tx_queues; i++) {
>> +			tx_ring[i].count = new_tx_count;
>> +			err = ixgbevf_setup_tx_resources(adapter,
>> +							 &tx_ring[i]);
>> +			if (err) {
>> +				while (i) {
>> +					i--;
>> +					ixgbevf_free_tx_resources(adapter,
>> +								  &tx_ring[i]);
>> +				}
>> +				goto err_setup;
>> +			}
>> +			tx_ring[i].v_idx = adapter->tx_ring[i].v_idx;
>> +		}
>> +		need_update = true;
>> +	}
>
>> +
>> +	rx_ring = kcalloc(adapter->num_rx_queues,
>> +			       sizeof(struct ixgbevf_ring), GFP_KERNEL);
>> +	if ((!rx_ring) && (need_update)) {
>> +		for (i = 0; i < adapter->num_tx_queues; i++)
>> +			ixgbevf_free_tx_resources(adapter, &tx_ring[i]);
>> +		kfree(tx_ring);
>> +		err = -ENOMEM;
>> +		goto err_setup;
>> +	}
>> +
>> +	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)
>
>
>> +			}
>> +			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?

Most definitely.  I'm fixing this up and will have it straightened out for the next revision of the driver patches.  Good catch again.

Regards,

- 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