| Message ID | 1879f6c8a7fcb5d7bb58ffb3d9fed26c8d7ec5cb.1665416630.git.sd@queasysnail.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | macsec: offload-related fixes | expand |
On Thu, Oct 13, 2022 at 04:15:41PM +0200, Sabrina Dubroca wrote: > secy->n_rx_sc is supposed to be the number of _active_ rxsc's within a > secy. This is then used by macsec_send_sci to help decide if we should > add the SCI to the header or not. > > This logic is currently broken when we create a new RXSC and turn it > off at creation, as create_rx_sc always sets ->active to true (and > immediately uses that to increment n_rx_sc), and only later > macsec_add_rxsc sets rx_sc->active. > > Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > --- > drivers/net/macsec.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c > index 0d6fe34b91ae..cdee342e42cd 100644 > --- a/drivers/net/macsec.c > +++ b/drivers/net/macsec.c > @@ -1413,7 +1413,8 @@ static struct macsec_rx_sc *del_rx_sc(struct macsec_secy *secy, sci_t sci) > return NULL; > } > > -static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci) > +static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci, > + bool active) > { > struct macsec_rx_sc *rx_sc; > struct macsec_dev *macsec; > @@ -1437,7 +1438,7 @@ static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci) > } > > rx_sc->sci = sci; > - rx_sc->active = true; > + rx_sc->active = active; > refcount_set(&rx_sc->refcnt, 1); > > secy = &macsec_priv(dev)->secy; > @@ -1876,6 +1877,7 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info) > struct macsec_rx_sc *rx_sc; > struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1]; > struct macsec_secy *secy; > + bool active = true; > int ret; > > if (!attrs[MACSEC_ATTR_IFINDEX]) > @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info) > secy = &macsec_priv(dev)->secy; > sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]); > > - rx_sc = create_rx_sc(dev, sci); > + > + if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) > + active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]); You don't need !! to assign to bool variables and can safely omit them. thanks > + > + rx_sc = create_rx_sc(dev, sci, active); > if (IS_ERR(rx_sc)) { > rtnl_unlock(); > return PTR_ERR(rx_sc); > } > > - if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) > - rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]); > - > if (macsec_is_offloaded(netdev_priv(dev))) { > const struct macsec_ops *ops; > struct macsec_context ctx; > -- > 2.38.0 >
2022-10-14, 09:16:56 +0300, Leon Romanovsky wrote: [...] > > @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info) > > secy = &macsec_priv(dev)->secy; > > sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]); > > > > - rx_sc = create_rx_sc(dev, sci); > > + > > + if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) > > + active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]); > > You don't need !! to assign to bool variables and can safely omit them. Yeah, but I'm just moving existing code, see below. (please trim your replies, nobody wants to scroll through endless quoting just to find one comment hiding in the middle) > > thanks > > > + > > + rx_sc = create_rx_sc(dev, sci, active); > > if (IS_ERR(rx_sc)) { > > rtnl_unlock(); > > return PTR_ERR(rx_sc); > > } > > > > - if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) > > - rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
On Fri, Oct 14, 2022 at 09:43:41AM +0200, Sabrina Dubroca wrote: > 2022-10-14, 09:16:56 +0300, Leon Romanovsky wrote: > [...] > > > @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info) > > > secy = &macsec_priv(dev)->secy; > > > sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]); > > > > > > - rx_sc = create_rx_sc(dev, sci); > > > + > > > + if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) > > > + active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]); > > > > You don't need !! to assign to bool variables and can safely omit them. > > Yeah, but I'm just moving existing code, see below. Not really, original code was "rx_sc->active = ...", but you changed to use local value. So it is perfectly fine to fix the !! too. Thanks
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 0d6fe34b91ae..cdee342e42cd 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -1413,7 +1413,8 @@ static struct macsec_rx_sc *del_rx_sc(struct macsec_secy *secy, sci_t sci) return NULL; } -static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci) +static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci, + bool active) { struct macsec_rx_sc *rx_sc; struct macsec_dev *macsec; @@ -1437,7 +1438,7 @@ static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci) } rx_sc->sci = sci; - rx_sc->active = true; + rx_sc->active = active; refcount_set(&rx_sc->refcnt, 1); secy = &macsec_priv(dev)->secy; @@ -1876,6 +1877,7 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info) struct macsec_rx_sc *rx_sc; struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1]; struct macsec_secy *secy; + bool active = true; int ret; if (!attrs[MACSEC_ATTR_IFINDEX]) @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info) secy = &macsec_priv(dev)->secy; sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]); - rx_sc = create_rx_sc(dev, sci); + + if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) + active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]); + + rx_sc = create_rx_sc(dev, sci, active); if (IS_ERR(rx_sc)) { rtnl_unlock(); return PTR_ERR(rx_sc); } - if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) - rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]); - if (macsec_is_offloaded(netdev_priv(dev))) { const struct macsec_ops *ops; struct macsec_context ctx;
secy->n_rx_sc is supposed to be the number of _active_ rxsc's within a secy. This is then used by macsec_send_sci to help decide if we should add the SCI to the header or not. This logic is currently broken when we create a new RXSC and turn it off at creation, as create_rx_sc always sets ->active to true (and immediately uses that to increment n_rx_sc), and only later macsec_add_rxsc sets rx_sc->active. Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- drivers/net/macsec.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)