netdev - Re: [PATCHv3 1/3] NETPOLL: Extend rx

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, 27 Feb 2012 15:33:55 -0800 (PST)
From:	Andrei Warkentin <awarkentin@...are.com>
To:	Jason Wessel <jason.wessel@...driver.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andrei Warkentin <andreiw@...are.com>,
	kgdb-bugreport@...ts.sourceforge.net,
	Matt Mackall <mpm@...enic.com>,
	Andrei Warkentin <andrey.warkentin@...il.com>
Subject: Re: [PATCHv3 1/3] NETPOLL: Extend rx_hook support.

Hi,

Thank you for the review, Jason. Comments inline.

----- Original Message -----
> From: "Jason Wessel" <jason.wessel@...driver.com>
> To: "Andrei Warkentin" <andrey.warkentin@...il.com>
> Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, "Andrei Warkentin" <andreiw@...are.com>,
> kgdb-bugreport@...ts.sourceforge.net, "Matt Mackall" <mpm@...enic.com>
> Sent: Monday, February 27, 2012 6:17:12 PM
> Subject: Re: [PATCHv3 1/3] NETPOLL: Extend rx_hook support.
> 
> 
> I am just now starting to look how this patch set compares to kgdboe.
>  For the kgdboe the patch is a bit different.  The kgdboe opted to
> just pass the skb so as to cut down on the number of arguments to
> the function call.
> 
> From the kgdboe patch:
> 
> -       void (*rx_hook)(struct netpoll *, int, char *, int);
> +       void (*rx_hook)(struct netpoll *, int, char *, int, struct
> sk_buff *);
> 
> 

Interesting, I thought about passing the skb, but decided I didn't
want to copy and paste the skb parsing code, especially given
that it's always UDP anyway. I still have reservations about
passing the physical address, but I don't think anyone tried
to use netpoll or a non-ethernet device anyway.

> >  
> > +void netpoll_poll_dev(struct net_device *dev);
> >  void netpoll_send_udp(struct netpoll *np, const char *msg, int
> >  
> > -static void netpoll_poll_dev(struct net_device *dev)
> > +void netpoll_poll_dev(struct net_device *dev)
> 
> 
> This is interesting and I will have to look into this further...  A
> large part of the reason kgdboe never went anywhere was all around
> the locking problems the ability to safely use the network hardware
> and restore the state when it was done.  It appears you made this
> change so as to make a lockless call directly instead of going
> through netpoll_poll().  I am not entirely sure you could safely do
> this.
> 
> In kgdboe we always had:
> 
> +static int eth_get_char(void)
> +{
> +	int chr;
> +
> +	while (atomic_read(&in_count) == 0)
> +		netpoll_poll(&np);
> 
> 
> If it is the case that you really can safely call the
> netpoll_poll_dev() without the locks then the horrible sync irq
> state etc... could go away in kgdboe, and then it would be worth
> considering digging up all the ethernet polling errata fixes that
> live of out the mainline and perhaps submit some for review.
> 

I didn't look deeply at kgdboe (probably should have...). Anyway, netpoll_poll
doesn't seem to exist. netpoll_poll_dev is called from netpoll_send_skb_on_dev
and the only contract I see is running with the interrupts disabled - something
that is satisfied by running in the context of KDB.

This is slight OT, but...are WiFi drivers sufficiently similar that netpoll "just works?"

Thanks again.

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