netdev - Re: [PATCH v7 net-next 2/4] net: Introduce generic failover module

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:   Fri, 20 Apr 2018 18:34:27 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Cc:     stephen@...workplumber.org, davem@...emloft.net,
        netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        virtio-dev@...ts.oasis-open.org, jesse.brandeburg@...el.com,
        alexander.h.duyck@...el.com, kubakici@...pl, jasowang@...hat.com,
        loseweigh@...il.com, jiri@...nulli.us
Subject: Re: [PATCH v7 net-next 2/4] net: Introduce generic failover module

On Fri, Apr 20, 2018 at 08:21:00AM -0700, Samudrala, Sridhar wrote:
> > > +	finfo = netdev_priv(failover_dev);
> > > +
> > > +	primary_dev = rtnl_dereference(finfo->primary_dev);
> > > +	standby_dev = rtnl_dereference(finfo->standby_dev);
> > > +
> > > +	if (slave_dev != primary_dev && slave_dev != standby_dev)
> > > +		goto done;
> > > +
> > > +	if ((primary_dev && failover_xmit_ready(primary_dev)) ||
> > > +	    (standby_dev && failover_xmit_ready(standby_dev))) {
> > > +		netif_carrier_on(failover_dev);
> > > +		netif_tx_wake_all_queues(failover_dev);
> > > +	} else {
> > > +		netif_carrier_off(failover_dev);
> > > +		netif_tx_stop_all_queues(failover_dev);
> > And I think it's a good idea to get stats from device here too.
> 
> Not sure why we need to get stats from lower devs here?

link down is often indication of a hardware problem.
lower dev might stop responding down the road.

> > > +static const struct net_device_ops failover_dev_ops = {
> > > +	.ndo_open		= failover_open,
> > > +	.ndo_stop		= failover_close,
> > > +	.ndo_start_xmit		= failover_start_xmit,
> > > +	.ndo_select_queue	= failover_select_queue,
> > > +	.ndo_get_stats64	= failover_get_stats,
> > > +	.ndo_change_mtu		= failover_change_mtu,
> > > +	.ndo_set_rx_mode	= failover_set_rx_mode,
> > > +	.ndo_validate_addr	= eth_validate_addr,
> > > +	.ndo_features_check	= passthru_features_check,
> > xdp support?
> 
> I think it should be possible to add it be calling the lower dev ndo_xdp routines
> with proper checks. can we add this later?

I'd be concerned that if you don't xdp userspace will keep poking
at lower devs. Then it will stop working if you add this later.

-- 
MST