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;
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
