Linux Virtualization - May 2018 - [PATCH net-next v10 2/4] net: Introduce generic failover module

On Mon,  7 May 2018 15:10:44 -0700
Sridhar Samudrala <sridhar.samudrala at intel.com> wrote:
> +static struct net_device *net_failover_get_bymac(u8 *mac,
> +						 struct net_failover_ops **ops)
> +{
> +	struct net_device *failover_dev;
> +	struct net_failover *failover;
> +
> +	spin_lock(&net_failover_lock);
> +	list_for_each_entry(failover, &net_failover_list, list) {
> +		failover_dev = rtnl_dereference(failover->failover_dev);
> +		if (ether_addr_equal(failover_dev->perm_addr, mac)) {
> +			*ops = rtnl_dereference(failover->ops);
> +			spin_unlock(&net_failover_lock);
> +			return failover_dev;
> +		}
> +	}
> +	spin_unlock(&net_failover_lock);
> +	return NULL;
> +}
This is broken if non-ethernet devices such as Infiniband are present.

On 5/7/2018 4:53 PM, Stephen Hemminger wrote:
> On Mon,  7 May 2018 15:10:44 -0700
> Sridhar Samudrala <sridhar.samudrala at intel.com> wrote:
>
>> +static struct net_device *net_failover_get_bymac(u8 *mac,
>> +						 struct net_failover_ops **ops)
>> +{
>> +	struct net_device *failover_dev;
>> +	struct net_failover *failover;
>> +
>> +	spin_lock(&net_failover_lock);
>> +	list_for_each_entry(failover, &net_failover_list, list) {
>> +		failover_dev = rtnl_dereference(failover->failover_dev);
>> +		if (ether_addr_equal(failover_dev->perm_addr, mac)) {
>> +			*ops = rtnl_dereference(failover->ops);
>> +			spin_unlock(&net_failover_lock);
>> +			return failover_dev;
>> +		}
>> +	}
>> +	spin_unlock(&net_failover_lock);
>> +	return NULL;
>> +}
> This is broken if non-ethernet devices such as Infiniband are present.
There is check to make sure that a slave and failover devices are of the same
type in
net_failover_slave_register()

	failover_dev = net_failover_get_bymac(slave_dev->perm_addr, &nfo_ops);
 ??????? if (!failover_dev)
 ??????????????? goto done;

 ??????? if (failover_dev->type != slave_dev->type)
 ??????????????? goto done;

Do you think this is not good enough? I had an explicit check for ARPHRD_ETHER
in
earlier patchsets, but removed it based on Jiri's comment.

On Mon, May 07, 2018 at 05:24:27PM -0700, Samudrala, Sridhar
wrote:
> 
> 
> On 5/7/2018 4:53 PM, Stephen Hemminger wrote:
> > On Mon,  7 May 2018 15:10:44 -0700
> > Sridhar Samudrala <sridhar.samudrala at intel.com> wrote:
> > 
> > > +static struct net_device *net_failover_get_bymac(u8 *mac,
> > > +						 struct net_failover_ops **ops)
> > > +{
> > > +	struct net_device *failover_dev;
> > > +	struct net_failover *failover;
> > > +
> > > +	spin_lock(&net_failover_lock);
> > > +	list_for_each_entry(failover, &net_failover_list, list) {
> > > +		failover_dev = rtnl_dereference(failover->failover_dev);
> > > +		if (ether_addr_equal(failover_dev->perm_addr, mac)) {
> > > +			*ops = rtnl_dereference(failover->ops);
> > > +			spin_unlock(&net_failover_lock);
> > > +			return failover_dev;
> > > +		}
> > > +	}
> > > +	spin_unlock(&net_failover_lock);
> > > +	return NULL;
> > > +}
> > This is broken if non-ethernet devices such as Infiniband are present.
> 
> There is check to make sure that a slave and failover devices are of the
same type in
> net_failover_slave_register()
> 
> 	failover_dev = net_failover_get_bymac(slave_dev->perm_addr,
&nfo_ops);
> ??????? if (!failover_dev)
> ??????????????? goto done;
> 
> ??????? if (failover_dev->type != slave_dev->type)
> ??????????????? goto done;
> 
> Do you think this is not good enough? I had an explicit check for
ARPHRD_ETHER in
> earlier patchsets, but removed it based on Jiri's comment.
Right but how is ether_addr_equal supposed to work if types are
identical but not ethernet?

This can also benefit from a comment referring to the check in
net_failover_slave_register.

-- 
MST