ovs_dp_change() stores the new features in dp->user_features before it updates the per-cpu upcall port IDs, and that update fails when the per-cpu-pids attribute is empty or its length is not a multiple of four. The request returns -EINVAL with the new features already recorded, while the code that enables or disables the tc skb extension key never ran. The datapath then claims TC_RECIRC_SHARING without holding the reference the bit stands for. A later request that clears the bit drops a reference that was never taken, and a datapath destroyed with the bit clear never drops the one it did take. tc_skb_ext_tc is one static key for the whole host. cls_flower and OVS take and drop references to it; the bridge, the flow dissector and the OVS flow path only read it, and they depend on the key staying on while they do. A count that is one short reaches zero while cls_flower still holds a reference, so recirc sharing in every datapath goes off with it, and so does the skb extension allocation the readers above depend on. A disable on a count of zero warns in static_key_dec_not_one() in kernel/jump_label.c, which panics a host that runs with panic_on_warn. The request only needs CAP_NET_ADMIN, which a user namespace that owns a network namespace provides. datapath_policy has no entry for OVS_DP_ATTR_PER_CPU_PIDS and the operation skips strict validation, so an empty attribute reaches the check that fails. Record the features last, in one setter that takes and drops the reference along with them, and call it from the places that change or clear the bit. Two of those places leaked a reference: a datapath that fails after ovs_dp_change() enabled the key was freed without a disable, and ovs_dp_reset_user_features() cleared the bit without one. The reset runs when an outdated user space instance sends OVS_DP_CMD_NEW for a name that already exists, and after it the datapath's packets no longer share the tc skb extension, which is what the warning it prints already announced. Fixes: 35d39fecbc24 ("net/sched: Enable tc skb ext allocation on chain miss only when needed") Cc: stable@vger.kernel.org Signed-off-by: Minxi Hou --- net/openvswitch/datapath.c | 34 +++++++++++++++++++++------------- net/openvswitch/datapath.h | 5 ++++- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index b2a0b09fd4a5..8fcb9f2c8ece 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -1704,6 +1704,20 @@ static struct datapath *lookup_datapath(struct net *net, return dp ? dp : ERR_PTR(-ENODEV); } +/* Called with ovs_mutex. */ +static void ovs_dp_set_user_features(struct datapath *dp, u32 features) +{ + bool was = dp->user_features & OVS_DP_F_TC_RECIRC_SHARING; + bool now = features & OVS_DP_F_TC_RECIRC_SHARING; + + if (now && !was) + tc_skb_ext_tc_enable(); + else if (!now && was) + tc_skb_ext_tc_disable(); + + dp->user_features = features; +} + static void ovs_dp_reset_user_features(struct sk_buff *skb, struct genl_info *info) { @@ -1716,7 +1730,7 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb, pr_warn("%s: Dropping previously announced user features\n", ovs_dp_name(dp)); - dp->user_features = 0; + ovs_dp_set_user_features(dp, 0); } static int ovs_dp_set_upcall_portids(struct datapath *dp, @@ -1773,7 +1787,7 @@ u32 ovs_dp_get_upcall_portid(const struct datapath *dp, uint32_t cpu_id) static int ovs_dp_change(struct datapath *dp, struct nlattr *a[]) { - u32 user_features = 0, old_features = dp->user_features; + u32 user_features = 0; int err; if (a[OVS_DP_ATTR_USER_FEATURES]) { @@ -1801,9 +1815,7 @@ static int ovs_dp_change(struct datapath *dp, struct nlattr *a[]) return err; } - dp->user_features = user_features; - - if (dp->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU && + if (user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU && a[OVS_DP_ATTR_PER_CPU_PIDS]) { /* Upcall Netlink Port IDs have been updated */ err = ovs_dp_set_upcall_portids(dp, @@ -1812,12 +1824,7 @@ static int ovs_dp_change(struct datapath *dp, struct nlattr *a[]) return err; } - if ((dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) && - !(old_features & OVS_DP_F_TC_RECIRC_SHARING)) - tc_skb_ext_tc_enable(); - else if (!(dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) && - (old_features & OVS_DP_F_TC_RECIRC_SHARING)) - tc_skb_ext_tc_disable(); + ovs_dp_set_user_features(dp, user_features); return 0; } @@ -1939,6 +1946,8 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info) err_destroy_portids: kfree(rcu_dereference_raw(dp->upcall_portids)); err_unlock_and_destroy_meters: + /* The datapath is freed here and never reaches __dp_destroy(). */ + ovs_dp_set_user_features(dp, 0); ovs_unlock(); ovs_meters_exit(dp); err_destroy_ports: @@ -1961,8 +1970,7 @@ static void __dp_destroy(struct datapath *dp) struct flow_table *table = &dp->table; int i; - if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) - tc_skb_ext_tc_disable(); + ovs_dp_set_user_features(dp, 0); for (i = 0; i < DP_VPORT_HASH_BUCKETS; i++) { struct vport *vport; diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h index 7a7afaeceee5..e94b77261b95 100644 --- a/net/openvswitch/datapath.h +++ b/net/openvswitch/datapath.h @@ -76,7 +76,10 @@ struct dp_nlsk_pids { * ovs_mutex and RCU. * @stats_percpu: Per-CPU datapath statistics. * @net: Reference to net namespace. - * @user_features: Bitmap of enabled %OVS_DP_F_* features. + * @user_features: Bitmap of enabled %OVS_DP_F_* features. A datapath + * that has %OVS_DP_F_TC_RECIRC_SHARING set holds one reference to the tc + * skb extension, so this field is written last, through + * ovs_dp_set_user_features(), with ovs_mutex held. * @max_headroom: The maximum headroom of all vports in this datapath; it will * be used by all the internal vports in this dp. * @meter_tbl: Meter table. -- 2.56.0