netdev - RE: [PATCH net 5/8] qlcnic: Fix reset recovery after transmit timeout

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] [day] [month] [year] [list]

Message-ID: <262CB373A6D1F14F9B81E82F74F77D5A4B3669@avmb2.qlogic.org>
Date:	Thu, 9 May 2013 06:21:58 +0000
From:	Shahed Shaikh <shahed.shaikh@...gic.com>
To:	David Miller <davem@...emloft.net>
CC:	netdev <netdev@...r.kernel.org>,
	Dept-NX Linux NIC Driver 
	<Dept_NX_Linux_NIC_Driver@...gic.com>,
	Sony Chacko <sony.chacko@...gic.com>
Subject: RE: [PATCH net 5/8] qlcnic: Fix reset recovery after transmit
 timeout


> -----Original Message-----
> From: David Miller [mailto:davem@...emloft.net]
> Sent: Thursday, May 09, 2013 12:37 AM
> To: Shahed Shaikh
> Cc: netdev; Dept-NX Linux NIC Driver; Sony Chacko
> Subject: Re: [PATCH net 5/8] qlcnic: Fix reset recovery after transmit timeout
> 
> From: Shahed Shaikh <shahed.shaikh@...gic.com>
> Date: Tue, 7 May 2013 15:55:03 -0400
> 
> > @@ -435,10 +435,7 @@ static void qlcnic_83xx_idc_attach_driver(struct
> qlcnic_adapter *adapter)
> >  	}
> >  done:
> >  	netif_device_attach(netdev);
> > -	if (netif_running(netdev)) {
> > -		netif_carrier_on(netdev);
> > -		netif_wake_queue(netdev);
> > -	}
> > +	adapter->netdev->trans_start = jiffies;
> >  }
> >
> >  static int qlcnic_83xx_idc_enter_failed_state(struct qlcnic_adapter
> > *adapter,
> 
> This is not right.
> 
> Multiqueue aware drivers should never access netdev->trans_start directly,
> and I see several such writes in this driver.
> 
> Anything you write here will be totally ignored by the rest of the kernel,
> because this value is overwritten by every call to dev_trans_start() which is
> the only valid method by which to determine this value.
> 
> dev_trans_start() walks all of the transmit queues, recording the most recent
> txq->trans_start timestamp in netdev->trans_start.
> 
> So if you write netdev->trans_start in your driver, it does nothing, because
> even the dev_watchdog() time is going to inspect the per-queue
> txq->trans_start values and ignore the netdev->trans_start value for
> devices whose driver suppports multiqueue as qlcnic does.

Hi David,

We will resubmit the current patch after removing access to netdev->trans_start.
We will fix the same issue in the rest of the driver code in a follow-up patch.

Thanks,
Shahed


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