[ovs-dev] [PATCH 05/11] datapath: Use nla_nest_start_noflag()
Yifeng Sun
pkusunyifeng at gmail.com
Mon Oct 14 23:35:36 UTC 2019
LGTM, thanks.
Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>
On Mon, Oct 14, 2019 at 10:53 AM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
> This patch backports the openvswitch changes and update the compat layer
> for the following upstream patch.
>
> commit ae0be8de9a53cda3505865c11826d8ff0640237c
> Author: Michal Kubecek <mkubecek at suse.cz>
> Date: Fri Apr 26 11:13:06 2019 +0200
>
> netlink: make nla_nest_start() add NLA_F_NESTED flag
>
> Even if the NLA_F_NESTED flag was introduced more than 11 years ago, most
> netlink based interfaces (including recently added ones) are still not
> setting it in kernel generated messages. Without the flag, message parsers
> not aware of attribute semantics (e.g. wireshark dissector or libmnl's
> mnl_nlmsg_fprintf()) cannot recognize nested attributes and won't display
> the structure of their contents.
>
> Unfortunately we cannot just add the flag everywhere as there may be
> userspace applications which check nlattr::nla_type directly rather than
> through a helper masking out the flags. Therefore the patch renames
> nla_nest_start() to nla_nest_start_noflag() and introduces nla_nest_start()
> as a wrapper adding NLA_F_NESTED. The calls which add NLA_F_NESTED manually
> are rewritten to use nla_nest_start().
>
> Except for changes in include/net/netlink.h, the patch was generated using
> this semantic patch:
>
> @@ expression E1, E2; @@
> -nla_nest_start(E1, E2)
> +nla_nest_start_noflag(E1, E2)
>
> @@ expression E1, E2; @@
> -nla_nest_start_noflag(E1, E2 | NLA_F_NESTED)
> +nla_nest_start(E1, E2)
>
> Signed-off-by: Michal Kubecek <mkubecek at suse.cz>
> Acked-by: Jiri Pirko <jiri at mellanox.com>
> Acked-by: David Ahern <dsahern at gmail.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
> acinclude.m4 | 1 +
> datapath/conntrack.c | 6 +++---
> datapath/datapath.c | 7 +++---
> datapath/flow_netlink.c | 33 +++++++++++++++--------------
> datapath/linux/compat/include/net/netlink.h | 9 ++++++++
> datapath/meter.c | 8 +++----
> datapath/vport-vxlan.c | 2 +-
> datapath/vport.c | 2 +-
> 8 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index dca09abefa96..fe121ab9126d 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -844,6 +844,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_put_in_addr])
> OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_find_nested])
> OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_is_last])
> + OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_nest_start_noflag])
> OVS_GREP_IFELSE([$KSRC/include/linux/netlink.h], [void.*netlink_set_err],
> [OVS_DEFINE([HAVE_VOID_NETLINK_SET_ERR])])
> OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netlink.h],
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 010f9af5ffd2..b11a30965147 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1776,7 +1776,7 @@ static bool ovs_ct_nat_to_attr(const struct ovs_conntrack_info *info,
> {
> struct nlattr *start;
>
> - start = nla_nest_start(skb, OVS_CT_ATTR_NAT);
> + start = nla_nest_start_noflag(skb, OVS_CT_ATTR_NAT);
> if (!start)
> return false;
>
> @@ -1847,7 +1847,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
> {
> struct nlattr *start;
>
> - start = nla_nest_start(skb, OVS_ACTION_ATTR_CT);
> + start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_CT);
> if (!start)
> return -EMSGSIZE;
>
> @@ -2257,7 +2257,7 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, struct genl_info *info)
> if (IS_ERR(reply))
> return PTR_ERR(reply);
>
> - nla_reply = nla_nest_start(reply, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
> + nla_reply = nla_nest_start_noflag(reply, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
>
> if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) {
> err = ovs_ct_limit_get_zone_limit(
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 94e4f6ffd6e9..78e2e6310529 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -475,7 +475,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>
>
> if (upcall_info->egress_tun_info) {
> - nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> + nla = nla_nest_start_noflag(user_skb,
> + OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> if (!nla) {
> err = -EMSGSIZE;
> goto out;
> @@ -487,7 +488,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
> }
>
> if (upcall_info->actions_len) {
> - nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_ACTIONS);
> + nla = nla_nest_start_noflag(user_skb, OVS_PACKET_ATTR_ACTIONS);
> if (!nla) {
> err = -EMSGSIZE;
> goto out;
> @@ -789,7 +790,7 @@ static int ovs_flow_cmd_fill_actions(const struct sw_flow *flow,
> * This can only fail for dump operations because the skb is always
> * properly sized for single flows.
> */
> - start = nla_nest_start(skb, OVS_FLOW_ATTR_ACTIONS);
> + start = nla_nest_start_noflag(skb, OVS_FLOW_ATTR_ACTIONS);
> if (start) {
> const struct sw_flow_actions *sf_acts;
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 0f7ab53fc141..35f13d753cec 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -839,7 +839,7 @@ static int vxlan_opt_to_nlattr(struct sk_buff *skb,
> const struct vxlan_metadata *opts = tun_opts;
> struct nlattr *nla;
>
> - nla = nla_nest_start(skb, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS);
> + nla = nla_nest_start_noflag(skb, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS);
> if (!nla)
> return -EMSGSIZE;
>
> @@ -926,7 +926,7 @@ static int ip_tun_to_nlattr(struct sk_buff *skb,
> struct nlattr *nla;
> int err;
>
> - nla = nla_nest_start(skb, OVS_KEY_ATTR_TUNNEL);
> + nla = nla_nest_start_noflag(skb, OVS_KEY_ATTR_TUNNEL);
> if (!nla)
> return -EMSGSIZE;
>
> @@ -1934,7 +1934,7 @@ static int nsh_key_to_nlattr(const struct ovs_key_nsh *nsh, bool is_mask,
> {
> struct nlattr *start;
>
> - start = nla_nest_start(skb, OVS_KEY_ATTR_NSH);
> + start = nla_nest_start_noflag(skb, OVS_KEY_ATTR_NSH);
> if (!start)
> return -EMSGSIZE;
>
> @@ -2017,14 +2017,15 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
> if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) {
> if (ovs_nla_put_vlan(skb, &output->eth.vlan, is_mask))
> goto nla_put_failure;
> - encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> + encap = nla_nest_start_noflag(skb, OVS_KEY_ATTR_ENCAP);
> if (!swkey->eth.vlan.tci)
> goto unencap;
>
> if (swkey->eth.cvlan.tci || eth_type_vlan(swkey->eth.type)) {
> if (ovs_nla_put_vlan(skb, &output->eth.cvlan, is_mask))
> goto nla_put_failure;
> - in_encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> + in_encap = nla_nest_start_noflag(skb,
> + OVS_KEY_ATTR_ENCAP);
> if (!swkey->eth.cvlan.tci)
> goto unencap;
> }
> @@ -2203,7 +2204,7 @@ int ovs_nla_put_key(const struct sw_flow_key *swkey,
> int err;
> struct nlattr *nla;
>
> - nla = nla_nest_start(skb, attr);
> + nla = nla_nest_start_noflag(skb, attr);
> if (!nla)
> return -EMSGSIZE;
> err = __ovs_nla_put_key(swkey, output, is_mask, skb);
> @@ -3234,7 +3235,7 @@ static int sample_action_to_attr(const struct nlattr *attr,
> const struct sample_arg *arg;
> struct nlattr *actions;
>
> - start = nla_nest_start(skb, OVS_ACTION_ATTR_SAMPLE);
> + start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_SAMPLE);
> if (!start)
> return -EMSGSIZE;
>
> @@ -3247,7 +3248,7 @@ static int sample_action_to_attr(const struct nlattr *attr,
> goto out;
> }
>
> - ac_start = nla_nest_start(skb, OVS_SAMPLE_ATTR_ACTIONS);
> + ac_start = nla_nest_start_noflag(skb, OVS_SAMPLE_ATTR_ACTIONS);
> if (!ac_start) {
> err = -EMSGSIZE;
> goto out;
> @@ -3273,7 +3274,7 @@ static int clone_action_to_attr(const struct nlattr *attr,
> struct nlattr *start;
> int err = 0, rem = nla_len(attr);
>
> - start = nla_nest_start(skb, OVS_ACTION_ATTR_CLONE);
> + start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_CLONE);
> if (!start)
> return -EMSGSIZE;
>
> @@ -3295,7 +3296,7 @@ static int check_pkt_len_action_to_attr(const struct nlattr *attr,
> const struct nlattr *a, *cpl_arg;
> int err = 0, rem = nla_len(attr);
>
> - start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
> + start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
> if (!start)
> return -EMSGSIZE;
>
> @@ -3314,8 +3315,8 @@ static int check_pkt_len_action_to_attr(const struct nlattr *attr,
> * 'OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL'.
> */
> a = nla_next(cpl_arg, &rem);
> - ac_start = nla_nest_start(skb,
> - OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
> + ac_start = nla_nest_start_noflag(skb,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
> if (!ac_start) {
> err = -EMSGSIZE;
> goto out;
> @@ -3333,8 +3334,8 @@ static int check_pkt_len_action_to_attr(const struct nlattr *attr,
> * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER.
> */
> a = nla_next(a, &rem);
> - ac_start = nla_nest_start(skb,
> - OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
> + ac_start = nla_nest_start_noflag(skb,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
> if (!ac_start) {
> err = -EMSGSIZE;
> goto out;
> @@ -3368,7 +3369,7 @@ static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
> struct ovs_tunnel_info *ovs_tun = nla_data(ovs_key);
> struct ip_tunnel_info *tun_info = &ovs_tun->tun_dst->u.tun_info;
>
> - start = nla_nest_start(skb, OVS_ACTION_ATTR_SET);
> + start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_SET);
> if (!start)
> return -EMSGSIZE;
>
> @@ -3400,7 +3401,7 @@ static int masked_set_action_to_set_action_attr(const struct nlattr *a,
> /* Revert the conversion we did from a non-masked set action to
> * masked set action.
> */
> - nla = nla_nest_start(skb, OVS_ACTION_ATTR_SET);
> + nla = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_SET);
> if (!nla)
> return -EMSGSIZE;
>
> diff --git a/datapath/linux/compat/include/net/netlink.h b/datapath/linux/compat/include/net/netlink.h
> index d42bf108b417..34fc3460dc81 100644
> --- a/datapath/linux/compat/include/net/netlink.h
> +++ b/datapath/linux/compat/include/net/netlink.h
> @@ -165,4 +165,13 @@ static inline int rpl_nla_parse(struct nlattr **tb, int maxtype,
> #define nla_parse rpl_nla_parse
> #endif
>
> +#ifndef HAVE_NLA_NEST_START_NOFLAG
> +static inline struct nlattr *rpl_nla_nest_start_noflag(struct sk_buff *skb,
> + int attrtype)
> +{
> + return nla_nest_start(skb, attrtype);
> +}
> +#define nla_nest_start_noflag rpl_nla_nest_start_noflag
> +#endif
> +
> #endif /* net/netlink.h */
> diff --git a/datapath/meter.c b/datapath/meter.c
> index eda14682fb96..b0a92891c7c0 100644
> --- a/datapath/meter.c
> +++ b/datapath/meter.c
> @@ -129,7 +129,7 @@ static int ovs_meter_cmd_reply_stats(struct sk_buff *reply, u32 meter_id,
> OVS_METER_ATTR_PAD))
> goto error;
>
> - nla = nla_nest_start(reply, OVS_METER_ATTR_BANDS);
> + nla = nla_nest_start_noflag(reply, OVS_METER_ATTR_BANDS);
> if (!nla)
> goto error;
>
> @@ -138,7 +138,7 @@ static int ovs_meter_cmd_reply_stats(struct sk_buff *reply, u32 meter_id,
> for (i = 0; i < meter->n_bands; ++i, ++band) {
> struct nlattr *band_nla;
>
> - band_nla = nla_nest_start(reply, OVS_BAND_ATTR_UNSPEC);
> + band_nla = nla_nest_start_noflag(reply, OVS_BAND_ATTR_UNSPEC);
> if (!band_nla || nla_put(reply, OVS_BAND_ATTR_STATS,
> sizeof(struct ovs_flow_stats),
> &band->stats))
> @@ -168,11 +168,11 @@ static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info)
> nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS))
> goto nla_put_failure;
>
> - nla = nla_nest_start(reply, OVS_METER_ATTR_BANDS);
> + nla = nla_nest_start_noflag(reply, OVS_METER_ATTR_BANDS);
> if (!nla)
> goto nla_put_failure;
>
> - band_nla = nla_nest_start(reply, OVS_BAND_ATTR_UNSPEC);
> + band_nla = nla_nest_start_noflag(reply, OVS_BAND_ATTR_UNSPEC);
> if (!band_nla)
> goto nla_put_failure;
> /* Currently only DROP band type is supported. */
> diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c
> index 05764467a687..70ed376e3869 100644
> --- a/datapath/vport-vxlan.c
> +++ b/datapath/vport-vxlan.c
> @@ -47,7 +47,7 @@ static int vxlan_get_options(const struct vport *vport, struct sk_buff *skb)
> #endif
> struct nlattr *exts;
>
> - exts = nla_nest_start(skb, OVS_TUNNEL_ATTR_EXTENSION);
> + exts = nla_nest_start_noflag(skb, OVS_TUNNEL_ATTR_EXTENSION);
> if (!exts)
> return -EMSGSIZE;
>
> diff --git a/datapath/vport.c b/datapath/vport.c
> index ed7f23ec8933..f929282dcec1 100644
> --- a/datapath/vport.c
> +++ b/datapath/vport.c
> @@ -408,7 +408,7 @@ int ovs_vport_get_options(const struct vport *vport, struct sk_buff *skb)
> if (!vport->ops->get_options)
> return 0;
>
> - nla = nla_nest_start(skb, OVS_VPORT_ATTR_OPTIONS);
> + nla = nla_nest_start_noflag(skb, OVS_VPORT_ATTR_OPTIONS);
> if (!nla)
> return -EMSGSIZE;
>
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list