netdev - Re: [PATCH v8 1/5] mac80211_hwsim: add PMSR capability support

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]

Date:   Wed, 8 Mar 2023 08:00:47 +0000
From:   Jaewan Kim <jaewan@...gle.com>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     gregkh@...uxfoundation.org, johannes@...solutions.net,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        kernel-team@...roid.com, adelva@...gle.com
Subject: Re: [PATCH v8 1/5] mac80211_hwsim: add PMSR capability support

On Mon, Mar 6, 2023 at 4:55 PM Simon Horman <simon.horman@...igine.com> wrote:
>
> On Thu, Mar 02, 2023 at 04:03:06PM +0000, Jaewan Kim wrote:
> > PMSR (a.k.a. peer measurement) is generalized measurement between two
> > Wi-Fi devices. And currently FTM (a.k.a. fine time measurement or flight
> > time measurement) is the one and only measurement. FTM is measured by
> > RTT (a.k.a. round trip time) of packets between two Wi-Fi devices.
> >
> > Add necessary functionality to allow mac80211_hwsim to be configured with
> > PMSR capability. The capability is mandatory to accept incoming PMSR
> > request because nl80211_pmsr_start() ignores incoming the request without
> > the PMSR capability.
> >
> > In detail, add new mac80211_hwsim attribute HWSIM_ATTR_PMSR_SUPPORT.
> > HWSIM_ATTR_PMSR_SUPPORT is used to set PMSR capability when creating a new
> > radio. To send extra capability details, HWSIM_ATTR_PMSR_SUPPORT can have
> > nested PMSR capability attributes defined in the nl80211.h. Data format is
> > the same as cfg80211_pmsr_capabilities.
> >
> > If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds
> > cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa.
> >
> > Signed-off-by: Jaewan Kim <jaewan@...gle.com>
>
> Thanks for your patch, a few comments below.
>
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index 4cc4eaf80b14..79476d55c1ca 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
>
> ...
>
> > @@ -3186,6 +3218,7 @@ struct hwsim_new_radio_params {
> >       u32 *ciphers;
> >       u8 n_ciphers;
> >       bool mlo;
> > +     const struct cfg80211_pmsr_capabilities *pmsr_capa;
>
> nit: not related to this patch,
>      but there are lots of holes in hwsim_new_radio_params.
>      And, I think that all fields, other than the new pmsr_capa field,
>      could fit into one cacheline on x86_64.
>
>      I'm unsure if it is worth cleaning up or not.
>
> >  };
> >
> >  static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
> > @@ -3260,7 +3293,7 @@ static int append_radio_msg(struct sk_buff *skb, int id,
> >                       return ret;
> >       }
> >
> > -     return 0;
> > +     return ret;
>
> This change seems unrelated to the rest of the patch.

I asked to change this in the prior patch v3.

>
> >  }
> >
> >  static void hwsim_mcast_new_radio(int id, struct genl_info *info,
>
> ...
>
> > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg80211_pmsr_capabilities *out,
> > +                        struct genl_info *info)
> > +{
> > +     struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
> > +     struct nlattr *nla;
> > +     int size;
>  +      int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
> > +                                hwsim_pmsr_capa_policy, NULL);
> > +
> > +     if (ret) {
> > +             NL_SET_ERR_MSG_ATTR(info->extack, pmsr_capa, "malformed PMSR capability");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> > +             out->max_peers = nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]);
> > +     out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
> > +     out->randomize_mac_addr = !!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR];
> > +
> > +     if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) {
> > +             NL_SET_ERR_MSG_ATTR(info->extack, tb[NL80211_PMSR_ATTR_TYPE_CAPA],
> > +                                 "malformed PMSR type");
> > +             return -EINVAL;
> > +     }
> > +
> > +     nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) {
> > +             switch (nla_type(nla)) {
> > +             case NL80211_PMSR_TYPE_FTM:
> > +                     parse_ftm_capa(nla, out, info);
> > +                     break;
> > +             default:
> > +                     WARN_ON(1);
>
> WARN_ON doesn't seem right here. I suspect that the following is more fitting.
>
>                 NL_SET_ERR_MSG_ATTR(...);
>                 return -EINVAL;
>

Not using NL_SET_ERR_MSG_ATTR(...) is intended to follow the pattern
of net/wireless/pmsr.c,
where unknown type isn't considered as an error.

-- 
Jaewan Kim (김재완) | Software Engineer in Google Korea |
jaewan@...gle.com | +82-10-2781-5078