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]
Date:	Thu, 02 May 2013 11:02:10 +0200
From:	Oliver Neukum <oneukum@...e.de>
To:	hayeswang <hayeswang@...ltek.com>
Cc:	gregkh@...uxfoundation.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	'nic_swsd' <nic_swsd@...ltek.com>
Subject: Re: [PATCH net-next] net/usb: new driver for RTL8152

On Thursday 02 May 2013 15:46:50 hayeswang wrote:
>  Oliver Neukum [mailto:oneukum@...e.de] 
> > Sent: Friday, April 26, 2013 7:57 PM
> > To: Hayeswang
> > Cc: gregkh@...uxfoundation.org; netdev@...r.kernel.org; 
> > linux-kernel@...r.kernel.org; linux-usb@...r.kernel.org; nic_swsd
> > Subject: Re: [PATCH net-next] net/usb: new driver for RTL8152
> > 
> [...]
> > > +static inline void set_ethernet_addr(struct r8152 *tp)
> > > +{
> > > +	struct net_device *dev = tp->netdev;
> > > +	u8 node_id[8] = {0};
> > > +
> > > +	if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) < 0)
> > 
> > DMA coherency rules. No buffers on the stack.
> 
> Excuse me. I don't understand what you mean. I reference the rtl8150.c and it

On some CPUs special operations need to be performed on buffers
intended for DMA. After these operations until the DMA is over any cacheline
shared with the buffer must not be touched. In addition it is not guaranteed
that the stack is DMAable at all.
This is documented in DMA-API.txt

I know this a very common bug because it works on x86, but on some architectures it
fails.

> uses the same way. Besides, when I check the other drivers, I find the data
> pointer of the parameter of the usb_control_msg() may be a pointer of a local
> variable from the other functions.
> 
> [...]
> > > +static void rtl_work_func_t(struct work_struct *work)
> > > +{
> > > +	struct r8152 *tp = container_of(work, struct r8152, 
> > schedule.work);
> > > +
> > > +	if (!netif_running(tp->netdev))
> > > +		goto out1;
> > > +
> > > +	if (test_bit(RTL8152_UNPLUG, &tp->flags))
> > > +		goto out1;
> > > +
> > > +	set_carrier(tp);
> > > +
> > > +	if (test_bit(RTL8152_SET_RX_MODE, &tp->flags))
> > > +		_rtl8152_set_rx_mode(tp->netdev);
> > > +
> > > +	schedule_delayed_work(&tp->schedule, HZ);
> > 
> > Why does this need to run permanently?
> 
> It is used to periodically check the speed, link status, and any other tasks
> which need to be finished.

Unfortunate. It will hurt power consumption.

> 
> [...]
> > > +static int rtl8152_close(struct net_device *netdev)
> > > +{
> > > +	struct r8152 *tp = netdev_priv(netdev);
> > > +	int res = 0;
> > > +
> > > +	cancel_delayed_work_sync(&tp->schedule);
> > 
> > Looks like a race. What makes sure the work isn't rescheduled?
> 
> netif_running would be checked.

I see.

	HTH
		Oliver

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

Powered by blists - more mailing lists