netdev - Re: [PATCH net-next v2 1/3] openvswitch: allow specifying ifindex of new interfaces

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]

Message-ID: <0989bea9-beb3-3eb1-eb55-3438f980d973@virtuozzo.com>
Date:   Tue, 23 Aug 2022 16:50:21 +0300
From:   Andrey Zhadchenko <andrey.zhadchenko@...tuozzo.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, dev@...nvswitch.org, pshelar@....org,
        davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
        ptikhomirov@...tuozzo.com, alexander.mikhalitsyn@...tuozzo.com,
        avagin@...gle.com, brauner@...nel.org, mark.d.gray@...hat.com,
        i.maximets@....org, aconole@...hat.com
Subject: Re: [PATCH net-next v2 1/3] openvswitch: allow specifying ifindex of
 new interfaces

Thanks for the review!

On 8/23/22 04:37, Jakub Kicinski wrote:
> On Fri, 19 Aug 2022 18:30:42 +0300 Andrey Zhadchenko wrote:
>> CRIU is preserving ifindexes of net devices after restoration. However,
>> current Open vSwitch API does not allow to target ifindex, so we cannot
>> correctly restore OVS configuration.
>>
>> Use ovs_header->dp_ifindex during OVS_DP_CMD_NEW as desired ifindex.
>> Use OVS_VPORT_ATTR_IFINDEX during OVS_VPORT_CMD_NEW to specify new netdev
>> ifindex.
> 
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -1739,6 +1739,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>   	struct vport *vport;
>>   	struct ovs_net *ovs_net;
>>   	int err;
>> +	struct ovs_header *ovs_header = info->userhdr;
>>   
>>   	err = -EINVAL;
>>   	if (!a[OVS_DP_ATTR_NAME] || !a[OVS_DP_ATTR_UPCALL_PID])
>> @@ -1779,6 +1780,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>   	parms.dp = dp;
>>   	parms.port_no = OVSP_LOCAL;
>>   	parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
>> +	parms.desired_ifindex = ovs_header->dp_ifindex;
> 
> Are you 100% sure that all user space making this call initializes
> dp_ifindex to 0? There is no validation in the kernel today that
> the field is not garbage as far as I can tell.
> 
> If you are sure, please add the appropriate analysis to the commit msg.

I am not sure that *all* users set dp_ifindex to zero. At least I do not 
know about them. Primary ovs userspace ovs-vswitchd certainly set to 
zero, others like WeaveNet do it too. But there can be more?
What is the policy regarding this? I can add a new attribute, it is not 
hard.

> 
>>   	/* So far only local changes have been made, now need the lock. */
>>   	ovs_lock();
>> @@ -2199,7 +2201,10 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>   	if (!a[OVS_VPORT_ATTR_NAME] || !a[OVS_VPORT_ATTR_TYPE] ||
>>   	    !a[OVS_VPORT_ATTR_UPCALL_PID])
>>   		return -EINVAL;
>> -	if (a[OVS_VPORT_ATTR_IFINDEX])
>> +
>> +	parms.type = nla_get_u32(a[OVS_VPORT_ATTR_TYPE]);
>> +
>> +	if (a[OVS_VPORT_ATTR_IFINDEX] && parms.type != OVS_VPORT_TYPE_INTERNAL)
>>   		return -EOPNOTSUPP;
>>   
>>   	port_no = a[OVS_VPORT_ATTR_PORT_NO]
>> @@ -2236,12 +2241,19 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>   	}
>>   
>>   	parms.name = nla_data(a[OVS_VPORT_ATTR_NAME]);
>> -	parms.type = nla_get_u32(a[OVS_VPORT_ATTR_TYPE]);
>>   	parms.options = a[OVS_VPORT_ATTR_OPTIONS];
>>   	parms.dp = dp;
>>   	parms.port_no = port_no;
>>   	parms.upcall_portids = a[OVS_VPORT_ATTR_UPCALL_PID];
>>   
>> +	if (parms.type == OVS_VPORT_TYPE_INTERNAL) {
>> +		if (a[OVS_VPORT_ATTR_IFINDEX])
> 
> You already validated that type must be internal for ifindex
> to be specified, so the outer if is unnecessary.
> 
> It's pretty common in netlink handling code to validate first
> then act assuming validation has passed.
> 
>> +			parms.desired_ifindex =
>> +				nla_get_u32(a[OVS_VPORT_ATTR_IFINDEX]);
>> +		else
>> +			parms.desired_ifindex = 0;
>> +	}
>> +
>>   	vport = new_vport(&parms);
>>   	err = PTR_ERR(vport);
>>   	if (IS_ERR(vport)) {
> 
>> diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
>> index 9de5030d9801..24e1cba2f1ac 100644
>> --- a/net/openvswitch/vport.h
>> +++ b/net/openvswitch/vport.h
>> @@ -98,6 +98,8 @@ struct vport_parms {
>>   	enum ovs_vport_type type;
>>   	struct nlattr *options;
>>   
>> +	int desired_ifindex;
> 
> Any chance this field would make sense somewhere else? you're adding
> a 4B field between two pointers it will result in a padding.
> 
> Also you're missing kdoc for this field.

Sure, will fix all the above in next version.

> 
>>   	/* For ovs_vport_alloc(). */
>>   	struct datapath *dp;
>>   	u16 port_no;
>