Re: [PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Alexander Aring <aahringo@redhat.com>]

Hi,

On Sun, Sep 25, 2022 at 6:27 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Wed, Sep 21, 2022 at 11:59 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Thu, 8 Sep 2022 21:00:37 -0400:
> >
> > > Hi,
> > >
> > > On Mon, Sep 5, 2022 at 4:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > Most of the PHYs seem to cope with the standard filtering rules by
> > > > default. Some of them might not, like hwsim which is only software, and
> > >
> > > yes, as I said before hwsim should pretend to be like all other
> > > hardware we have.
> > >
> > > > in this case advertises its real filtering level with the new
> > > > "filtering" internal value.
> > > >
> > > > The core then needs to check what is expected by looking at the PHY
> > > > requested filtering level and possibly apply additional filtering
> > > > rules.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  include/net/ieee802154_netdev.h |  8 ++++
> > > >  net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 86 insertions(+)
> > > >
> > > > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> > > > index d0d188c3294b..1b82bbafe8c7 100644
> > > > --- a/include/net/ieee802154_netdev.h
> > > > +++ b/include/net/ieee802154_netdev.h
> > > > @@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
> > > >  #endif
> > > >  };
> > > >
> > > > +enum ieee802154_frame_version {
> > > > +       IEEE802154_2003_STD,
> > > > +       IEEE802154_2006_STD,
> > > > +       IEEE802154_STD,
> > > > +       IEEE802154_RESERVED_STD,
> > > > +       IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
> > > > +};
> > > > +
> > > >  struct ieee802154_hdr {
> > > >         struct ieee802154_hdr_fc fc;
> > > >         u8 seq;
> > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > > index c43289c0fdd7..bc46e4a7669d 100644
> > > > --- a/net/mac802154/rx.c
> > > > +++ b/net/mac802154/rx.c
> > > > @@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> > > >                                 mac_cb(skb)->type);
> > > >                         goto fail;
> > > >                 }
> > > > +       } else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&
> > >
> > > We switch here from determine that receive path, means way we are
> > > going from interface type to the required filtering value. Sure there
> > > is currently a 1:1 mapping for them now but I don't know why we are
> > > doing that and this is in my opinion wrong. The receive path should
> > > depend on interface type as it was before and for scanning there is
> > > some early check like:
> >
> > Maybe on this one I am not fully convinced yet.
> >
> > In your opinion (I try to rephrase so that we align on what you told
> > me) the total lack of filtering is only something that is reserved to
> > monitor interfaces, so you make an implicit link between interface type
> > and filtering level.
>
> it always depends on the use case, but in the sense of filtering-level
> in "normal" operating mode and calling netif_skb_deliver_foo(), yes.
>
> The use case for e.g. scan is different and mac802154 takes control of it.
>
> >
> > I would argue that this is true today, but as the "no filtering at all"
> > level is defined in the spec, I assumed it was a possible level that
> > one would want to achieve some day (not sure for what purpose yet). So
> > I assumed it would be more relevant to only work with the
> > expected filtering level in the receive path rather than on the
> > interface type, it makes more sense IMHO. In practice I agree it should
> > be the same filtering-wise, but from a conceptual point of view I find
> > the current logic partially satisfying.
> >
>
> I don't quite follow here. I would say we currently only support to
> tell the hardware the whole filtering level (with AACK support) or the
> non-filtering level. With both we should somehow able to support
> interface types which requires
>
> > Would you agree with me only using "expected filtering levels" rather
> > than:
> > - sometimes the interface type
> > - sometimes the mac state (scan)
> > - otherwise, by default, the highest filtering level
> > ?
>
> I think so, yes? I don't know what "otherwise, by default, the highest
> filtering level" means, it is the interface type which declares what
> it's actually needs at netif_skb_deliver_foo(), e.g. monitors will
> call netif_skb_deliver_foo() even without AACK support... because
> that's how they working. They also don't have an address in the

they don't have an address -> the hardware filter is set to invalid
destination address setting and this should always be set when
switching to a mode which disables address filter. In case of your
scan command it should be then switched back.

- Alex