Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Alexander,

aahringo@redhat.com wrote on Mon, 10 Oct 2022 21:21:17 -0400:

> Hi,
> 
> On Mon, Oct 10, 2022 at 9:13 PM Alexander Aring <aahringo@redhat.com> wrote:
> >
> > Hi,
> >
> > On Mon, Oct 10, 2022 at 9:04 PM Alexander Aring <aahringo@redhat.com> wrote:  
> > >
> > > Hi,
> > >
> > > On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > We have access to the address filters being theoretically applied, we
> > > > also have access to the actual filtering level applied, so let's add a
> > > > proper frame validation sequence in hwsim.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
> > > >  include/net/ieee802154_netdev.h          |   8 ++
> > > >  2 files changed, 117 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > index 458be66b5195..84ee948f35bc 100644
> > > > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > > > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > @@ -18,6 +18,7 @@
> > > >  #include <linux/netdevice.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/spinlock.h>
> > > > +#include <net/ieee802154_netdev.h>
> > > >  #include <net/mac802154.h>
> > > >  #include <net/cfg802154.h>
> > > >  #include <net/genetlink.h>
> > > > @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
> > > >         return 0;
> > > >  }
> > > >
> > > > +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> > > > +                            u8 lqi)
> > > > +{
> > > > +       struct ieee802154_hdr hdr;
> > > > +       struct hwsim_phy *phy = hw->priv;
> > > > +       struct hwsim_pib *pib;
> > > > +
> > > > +       rcu_read_lock();
> > > > +       pib = rcu_dereference(phy->pib);
> > > > +
> > > > +       if (!pskb_may_pull(skb, 3)) {
> > > > +               dev_dbg(hw->parent, "invalid frame\n");
> > > > +               goto drop;
> > > > +       }
> > > > +
> > > > +       memcpy(&hdr, skb->data, 3);
> > > > +
> > > > +       /* Level 4 filtering: Frame fields validity */
> > > > +       if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {  
> >
> > I see, there is this big if handling. But it accesses the
> > hw->phy->filtering value. It should be part of the hwsim pib setting
> > set by the driver callback. It is a question here of mac802154 layer
> > setting vs driver layer setting. We should do what the mac802154 tells
> > the driver to do, this way we do what the mac802154 layer is set to.
> >
> > However it's a minor thing and it's okay to do it so...  
> 
> * whereas we never let the driver know at any time of what different
> filter levels exist _currently_ we have only the promiscuous mode
> on/off switch which is do nothing or 4_FRAME_FIELDS.
> It will work for now, changing anything in the mac802154 filtering
> fields or something will end in probably breakage in this handling. In
> my point of view as the current state is it should not do that, as
> remember that hwsim will "simulate" hardware it should not be able to
> access mac802154 fields (especially when doing receiving of frames) as
> other hardware will only set register bits (as hwsim pib values is
> there for)...
> 
> Still I think it's fine for now.

I see your point, indeed I could have added another PIB attribute
instead of accessing the PHY state.

I am fine doing it in a followup patch if this what you prefer. Shall I
do it?

Thanks,
Miquèl