| Message ID | 20221207101017.533-1-ehakim@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink | expand |
Wed, Dec 07, 2022 at 11:10:16AM CET, ehakim@nvidia.com wrote: >From: Emeel Hakim <ehakim@nvidia.com> > >Add support for changing Macsec offload selection through the >netlink layer by implementing the relevant changes in >macsec_change link. > >Since the handling in macsec_changelink is similar to macsec_upd_offload, >update macsec_upd_offload to use a common helper function to avoid >duplication. > >Example for setting offload for a macsec device: > ip link set macsec0 type macsec offload mac > >Reviewed-by: Raed Salem <raeds@nvidia.com> >Signed-off-by: Emeel Hakim <ehakim@nvidia.com> >--- >V2 -> V3: - Split the original patch into 3 patches, the macsec_rtnl_policy related change (separate patch) > to be sent to "net" branch as a fix. > - Change the original patch title to make it clear that it's only adding IFLA_MACSEC_OFFLOAD > to changelink >V1 -> V2: Add common helper to avoid duplicating code > > drivers/net/macsec.c | 102 +++++++++++++++++++++++++++---------------- > 1 file changed, 64 insertions(+), 38 deletions(-) > >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c >index d73b9d535b7a..1850a1ee4380 100644 >--- a/drivers/net/macsec.c >+++ b/drivers/net/macsec.c >@@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev *macsec) > return false; > } > >+static int macsec_update_offload(struct macsec_dev *macsec, enum macsec_offload offload) >+{ >+ enum macsec_offload prev_offload; >+ const struct macsec_ops *ops; >+ struct macsec_context ctx; >+ int ret = 0; >+ >+ prev_offload = macsec->offload; >+ >+ /* Check if the device already has rules configured: we do not support >+ * rules migration. >+ */ >+ if (macsec_is_configured(macsec)) >+ return -EBUSY; >+ >+ ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, >+ macsec, &ctx); >+ if (!ops) >+ return -EOPNOTSUPP; >+ >+ macsec->offload = offload; >+ >+ ctx.secy = &macsec->secy; >+ ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops->mdo_del_secy, &ctx) : >+ macsec_offload(ops->mdo_add_secy, &ctx); >+ >+ if (ret) >+ macsec->offload = prev_offload; >+ >+ return ret; >+} >+ > static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) > { > struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1]; >- enum macsec_offload offload, prev_offload; >- int (*func)(struct macsec_context *ctx); > struct nlattr **attrs = info->attrs; >- struct net_device *dev; >- const struct macsec_ops *ops; >- struct macsec_context ctx; >+ enum macsec_offload offload; > struct macsec_dev *macsec; >+ struct net_device *dev; > int ret; > > if (!attrs[MACSEC_ATTR_IFINDEX]) >@@ -2629,39 +2658,7 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) > > rtnl_lock(); > >- prev_offload = macsec->offload; >- macsec->offload = offload; >- >- /* Check if the device already has rules configured: we do not support >- * rules migration. >- */ >- if (macsec_is_configured(macsec)) { >- ret = -EBUSY; >- goto rollback; >- } >- >- ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, >- macsec, &ctx); >- if (!ops) { >- ret = -EOPNOTSUPP; >- goto rollback; >- } >- >- if (prev_offload == MACSEC_OFFLOAD_OFF) >- func = ops->mdo_add_secy; >- else >- func = ops->mdo_del_secy; >- >- ctx.secy = &macsec->secy; >- ret = macsec_offload(func, &ctx); >- if (ret) >- goto rollback; >- >- rtnl_unlock(); >- return 0; >- >-rollback: >- macsec->offload = prev_offload; >+ ret = macsec_update_offload(macsec, offload); > > rtnl_unlock(); > return ret; >@@ -3803,6 +3800,29 @@ static int macsec_changelink_common(struct net_device *dev, > return 0; > } > >+static int macsec_changelink_upd_offload(struct net_device *dev, struct nlattr *data[]) Only data[IFLA_MACSEC_OFFLOAD] is used there. Pass just this attr. >+{ >+ enum macsec_offload offload; >+ struct macsec_dev *macsec; >+ >+ macsec = macsec_priv(dev); >+ offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); >+ >+ if (macsec->offload == offload) >+ return 0; >+ >+ /* Check if the offloading mode is supported by the underlying layers */ >+ if (offload != MACSEC_OFFLOAD_OFF && >+ !macsec_check_offload(offload, macsec)) >+ return -EOPNOTSUPP; >+ >+ /* Check if the net device is busy. */ >+ if (netif_running(dev)) >+ return -EBUSY; >+ >+ return macsec_update_offload(macsec, offload); >+} >+ > static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], > struct nlattr *data[], > struct netlink_ext_ack *extack) >@@ -3831,6 +3851,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], > if (ret) > goto cleanup; > >+ if (data[IFLA_MACSEC_OFFLOAD]) { >+ ret = macsec_changelink_upd_offload(dev, data); >+ if (ret) >+ goto cleanup; >+ } >+ > /* If h/w offloading is available, propagate to the device */ > if (macsec_is_offloaded(macsec)) { > const struct macsec_ops *ops; >-- >2.21.3 >
2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote: > From: Emeel Hakim <ehakim@nvidia.com> > > Add support for changing Macsec offload selection through the > netlink layer by implementing the relevant changes in > macsec_change link. nit: macsec_changelink [...] > +static int macsec_update_offload(struct macsec_dev *macsec, enum macsec_offload offload) > +{ > + enum macsec_offload prev_offload; > + const struct macsec_ops *ops; > + struct macsec_context ctx; > + int ret = 0; > + > + prev_offload = macsec->offload; > + > + /* Check if the device already has rules configured: we do not support > + * rules migration. > + */ > + if (macsec_is_configured(macsec)) > + return -EBUSY; > + > + ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, > + macsec, &ctx); > + if (!ops) > + return -EOPNOTSUPP; > + > + macsec->offload = offload; > + > + ctx.secy = &macsec->secy; > + ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops->mdo_del_secy, &ctx) : > + macsec_offload(ops->mdo_add_secy, &ctx); I think aligning the two macsec_offload(...) calls would make this a bit easier to read: ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx) : macsec_offload(ops->mdo_add_secy, &ctx); (and remove the unnecessary ()) > + > + if (ret) > + macsec->offload = prev_offload; > + > + return ret; > +} > + [...] > +static int macsec_changelink_upd_offload(struct net_device *dev, struct nlattr *data[]) > +{ > + enum macsec_offload offload; > + struct macsec_dev *macsec; > + > + macsec = macsec_priv(dev); > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); All those checks are also present in macsec_upd_offload, why not move them into macsec_update_offload as well? (and then you don't really need macsec_changelink_upd_offload anymore) > + if (macsec->offload == offload) > + return 0; > + > + /* Check if the offloading mode is supported by the underlying layers */ > + if (offload != MACSEC_OFFLOAD_OFF && > + !macsec_check_offload(offload, macsec)) > + return -EOPNOTSUPP; > + > + /* Check if the net device is busy. */ > + if (netif_running(dev)) > + return -EBUSY; > + > + return macsec_update_offload(macsec, offload); > +} > +
> -----Original Message----- > From: Sabrina Dubroca <sd@queasysnail.net> > Sent: Wednesday, 7 December 2022 17:46 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; jiri@resnulli.us > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > External email: Use caution opening links or attachments > > > 2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote: > > From: Emeel Hakim <ehakim@nvidia.com> > > > > Add support for changing Macsec offload selection through the netlink > > layer by implementing the relevant changes in macsec_change link. > > nit: macsec_changelink Ack > [...] > > +static int macsec_update_offload(struct macsec_dev *macsec, enum > > +macsec_offload offload) { > > + enum macsec_offload prev_offload; > > + const struct macsec_ops *ops; > > + struct macsec_context ctx; > > + int ret = 0; > > + > > + prev_offload = macsec->offload; > > + > > + /* Check if the device already has rules configured: we do not support > > + * rules migration. > > + */ > > + if (macsec_is_configured(macsec)) > > + return -EBUSY; > > + > > + ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : > offload, > > + macsec, &ctx); > > + if (!ops) > > + return -EOPNOTSUPP; > > + > > + macsec->offload = offload; > > + > > + ctx.secy = &macsec->secy; > > + ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops- > >mdo_del_secy, &ctx) : > > + macsec_offload(ops->mdo_add_secy, &ctx); > > I think aligning the two macsec_offload(...) calls would make this a bit easier to > read: > > ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops- > >mdo_del_secy, &ctx) > : macsec_offload(ops->mdo_add_secy, &ctx); > > (and remove the unnecessary ()) Ack > > + > > + if (ret) > > + macsec->offload = prev_offload; > > + > > + return ret; > > +} > > + > > [...] > > +static int macsec_changelink_upd_offload(struct net_device *dev, > > +struct nlattr *data[]) { > > + enum macsec_offload offload; > > + struct macsec_dev *macsec; > > + > > + macsec = macsec_priv(dev); > > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > > All those checks are also present in macsec_upd_offload, why not move them into > macsec_update_offload as well? (and then you don't really need > macsec_changelink_upd_offload anymore) > Right, I thought about it , but I realized that those checks are done before holding the lock in macsec_upd_offload and if I move them to macsec_update_offload I will hold the lock for a longer time , I want to minimize the time of holding the lock. > > + if (macsec->offload == offload) > > + return 0; > > + > > + /* Check if the offloading mode is supported by the underlying layers */ > > + if (offload != MACSEC_OFFLOAD_OFF && > > + !macsec_check_offload(offload, macsec)) > > + return -EOPNOTSUPP; > > + > > + /* Check if the net device is busy. */ > > + if (netif_running(dev)) > > + return -EBUSY; > > + > > + return macsec_update_offload(macsec, offload); } > > + > > -- > Sabrina
2022-12-07, 15:52:15 +0000, Emeel Hakim wrote: > > > > -----Original Message----- > > From: Sabrina Dubroca <sd@queasysnail.net> > > Sent: Wednesday, 7 December 2022 17:46 > > To: Emeel Hakim <ehakim@nvidia.com> > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; jiri@resnulli.us > > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > External email: Use caution opening links or attachments > > > > > > 2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote: > > [...] > > > +static int macsec_changelink_upd_offload(struct net_device *dev, > > > +struct nlattr *data[]) { > > > + enum macsec_offload offload; > > > + struct macsec_dev *macsec; > > > + > > > + macsec = macsec_priv(dev); > > > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > > > > All those checks are also present in macsec_upd_offload, why not move them into > > macsec_update_offload as well? (and then you don't really need > > macsec_changelink_upd_offload anymore) > > > > Right, I thought about it , but I realized that those checks are done before holding the lock in macsec_upd_offload > and if I move them to macsec_update_offload I will hold the lock for a longer time , I want to minimize the time > of holding the lock. Those couple of tests are probably lost in the noise compared to what mdo_add_secy ends up doing. It also looks like a race condition between the "macsec->offload == offload" test in macsec_upd_offload (outside rtnl_lock) and updating macsec->offload via macsec_changelink is possible. (Currently we can only change it with macsec_upd_offload (called under genl_lock) so there's no issue until we add this patch) > > > + if (macsec->offload == offload) > > > + return 0; > > > + > > > + /* Check if the offloading mode is supported by the underlying layers */ > > > + if (offload != MACSEC_OFFLOAD_OFF && > > > + !macsec_check_offload(offload, macsec)) > > > + return -EOPNOTSUPP; > > > + > > > + /* Check if the net device is busy. */ > > > + if (netif_running(dev)) > > > + return -EBUSY; > > > + > > > + return macsec_update_offload(macsec, offload); } > > > + > > > > -- > > Sabrina >
> -----Original Message----- > From: Sabrina Dubroca <sd@queasysnail.net> > Sent: Thursday, 8 December 2022 0:04 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; jiri@resnulli.us > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > External email: Use caution opening links or attachments > > > 2022-12-07, 15:52:15 +0000, Emeel Hakim wrote: > > > > > > > -----Original Message----- > > > From: Sabrina Dubroca <sd@queasysnail.net> > > > Sent: Wednesday, 7 December 2022 17:46 > > > To: Emeel Hakim <ehakim@nvidia.com> > > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; > > > jiri@resnulli.us > > > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > > > External email: Use caution opening links or attachments > > > > > > > > > 2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote: > > > [...] > > > > +static int macsec_changelink_upd_offload(struct net_device *dev, > > > > +struct nlattr *data[]) { > > > > + enum macsec_offload offload; > > > > + struct macsec_dev *macsec; > > > > + > > > > + macsec = macsec_priv(dev); > > > > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > > > > > > All those checks are also present in macsec_upd_offload, why not > > > move them into macsec_update_offload as well? (and then you don't > > > really need macsec_changelink_upd_offload anymore) > > > > > > > Right, I thought about it , but I realized that those checks are done > > before holding the lock in macsec_upd_offload and if I move them to > > macsec_update_offload I will hold the lock for a longer time , I want to minimize > the time of holding the lock. > > Those couple of tests are probably lost in the noise compared to what > mdo_add_secy ends up doing. It also looks like a race condition between the > "macsec->offload == offload" test in macsec_upd_offload (outside rtnl_lock) and > updating macsec->offload via macsec_changelink is possible. (Currently we can > only change it with macsec_upd_offload (called under genl_lock) so there's no issue > until we add this patch) Ack, so getting rid of macsec_changelink_upd_offload and moving the locking inside macsec_update_offload should handle this issue > > > > > + if (macsec->offload == offload) > > > > + return 0; > > > > + > > > > + /* Check if the offloading mode is supported by the underlying layers */ > > > > + if (offload != MACSEC_OFFLOAD_OFF && > > > > + !macsec_check_offload(offload, macsec)) > > > > + return -EOPNOTSUPP; > > > > + > > > > + /* Check if the net device is busy. */ > > > > + if (netif_running(dev)) > > > > + return -EBUSY; > > > > + > > > > + return macsec_update_offload(macsec, offload); } > > > > + > > > > > > -- > > > Sabrina > > > > -- > Sabrina
2022-12-08, 06:53:18 +0000, Emeel Hakim wrote: > > > > -----Original Message----- > > From: Sabrina Dubroca <sd@queasysnail.net> > > Sent: Thursday, 8 December 2022 0:04 > > To: Emeel Hakim <ehakim@nvidia.com> > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; jiri@resnulli.us > > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > External email: Use caution opening links or attachments > > > > > > 2022-12-07, 15:52:15 +0000, Emeel Hakim wrote: > > > > > > > > > > -----Original Message----- > > > > From: Sabrina Dubroca <sd@queasysnail.net> > > > > Sent: Wednesday, 7 December 2022 17:46 > > > > To: Emeel Hakim <ehakim@nvidia.com> > > > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > > > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; > > > > jiri@resnulli.us > > > > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > > > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > 2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote: > > > > [...] > > > > > +static int macsec_changelink_upd_offload(struct net_device *dev, > > > > > +struct nlattr *data[]) { > > > > > + enum macsec_offload offload; > > > > > + struct macsec_dev *macsec; > > > > > + > > > > > + macsec = macsec_priv(dev); > > > > > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > > > > > > > > All those checks are also present in macsec_upd_offload, why not > > > > move them into macsec_update_offload as well? (and then you don't > > > > really need macsec_changelink_upd_offload anymore) > > > > > > > > > > Right, I thought about it , but I realized that those checks are done > > > before holding the lock in macsec_upd_offload and if I move them to > > > macsec_update_offload I will hold the lock for a longer time , I want to minimize > > the time of holding the lock. > > > > Those couple of tests are probably lost in the noise compared to what > > mdo_add_secy ends up doing. It also looks like a race condition between the > > "macsec->offload == offload" test in macsec_upd_offload (outside rtnl_lock) and > > updating macsec->offload via macsec_changelink is possible. (Currently we can > > only change it with macsec_upd_offload (called under genl_lock) so there's no issue > > until we add this patch) > > Ack, > so getting rid of macsec_changelink_upd_offload and moving the locking inside macsec_update_offload > should handle this issue You mean moving rtnl_lock()/unlock inside macsec_update_offload? changelink is already under rtnl_lock. Just move the checks that you currently have in macsec_changelink_upd_offload into macsec_update_offload, and remove them from macsec_upd_offload. > > > > > > > + if (macsec->offload == offload) > > > > > + return 0; > > > > > + > > > > > + /* Check if the offloading mode is supported by the underlying layers */ > > > > > + if (offload != MACSEC_OFFLOAD_OFF && > > > > > + !macsec_check_offload(offload, macsec)) > > > > > + return -EOPNOTSUPP; > > > > > + > > > > > + /* Check if the net device is busy. */ > > > > > + if (netif_running(dev)) > > > > > + return -EBUSY; > > > > > + > > > > > + return macsec_update_offload(macsec, offload); } > > > > > + > > > > > > > > -- > > > > Sabrina > > > > > > > -- > > Sabrina >
> -----Original Message----- > From: Sabrina Dubroca <sd@queasysnail.net> > Sent: Thursday, 8 December 2022 12:38 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; jiri@resnulli.us > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > External email: Use caution opening links or attachments > > > 2022-12-08, 06:53:18 +0000, Emeel Hakim wrote: > > > > > > > -----Original Message----- > > > From: Sabrina Dubroca <sd@queasysnail.net> > > > Sent: Thursday, 8 December 2022 0:04 > > > To: Emeel Hakim <ehakim@nvidia.com> > > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; > > > jiri@resnulli.us > > > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > > > External email: Use caution opening links or attachments > > > > > > > > > 2022-12-07, 15:52:15 +0000, Emeel Hakim wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Sabrina Dubroca <sd@queasysnail.net> > > > > > Sent: Wednesday, 7 December 2022 17:46 > > > > > To: Emeel Hakim <ehakim@nvidia.com> > > > > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > > > > pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; > > > > > jiri@resnulli.us > > > > > Subject: Re: [PATCH net-next v3 1/2] macsec: add support for > > > > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > 2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote: > > > > > [...] > > > > > > +static int macsec_changelink_upd_offload(struct net_device > > > > > > +*dev, struct nlattr *data[]) { > > > > > > + enum macsec_offload offload; > > > > > > + struct macsec_dev *macsec; > > > > > > + > > > > > > + macsec = macsec_priv(dev); > > > > > > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > > > > > > > > > > All those checks are also present in macsec_upd_offload, why not > > > > > move them into macsec_update_offload as well? (and then you > > > > > don't really need macsec_changelink_upd_offload anymore) > > > > > > > > > > > > > Right, I thought about it , but I realized that those checks are > > > > done before holding the lock in macsec_upd_offload and if I move > > > > them to macsec_update_offload I will hold the lock for a longer > > > > time , I want to minimize > > > the time of holding the lock. > > > > > > Those couple of tests are probably lost in the noise compared to > > > what mdo_add_secy ends up doing. It also looks like a race condition > > > between the "macsec->offload == offload" test in macsec_upd_offload > > > (outside rtnl_lock) and updating macsec->offload via > > > macsec_changelink is possible. (Currently we can only change it with > > > macsec_upd_offload (called under genl_lock) so there's no issue > > > until we add this patch) > > > > Ack, > > so getting rid of macsec_changelink_upd_offload and moving the locking > > inside macsec_update_offload should handle this issue > > You mean moving rtnl_lock()/unlock inside macsec_update_offload? > changelink is already under rtnl_lock. Just move the checks that you currently have > in macsec_changelink_upd_offload into macsec_update_offload, and remove them > from macsec_upd_offload. Ack will send new version > > > > > > > > > + if (macsec->offload == offload) > > > > > > + return 0; > > > > > > + > > > > > > + /* Check if the offloading mode is supported by the underlying layers > */ > > > > > > + if (offload != MACSEC_OFFLOAD_OFF && > > > > > > + !macsec_check_offload(offload, macsec)) > > > > > > + return -EOPNOTSUPP; > > > > > > + > > > > > > + /* Check if the net device is busy. */ > > > > > > + if (netif_running(dev)) > > > > > > + return -EBUSY; > > > > > > + > > > > > > + return macsec_update_offload(macsec, offload); } > > > > > > + > > > > > > > > > > -- > > > > > Sabrina > > > > > > > > > > -- > > > Sabrina > > > > -- > Sabrina
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index d73b9d535b7a..1850a1ee4380 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev *macsec) return false; } +static int macsec_update_offload(struct macsec_dev *macsec, enum macsec_offload offload) +{ + enum macsec_offload prev_offload; + const struct macsec_ops *ops; + struct macsec_context ctx; + int ret = 0; + + prev_offload = macsec->offload; + + /* Check if the device already has rules configured: we do not support + * rules migration. + */ + if (macsec_is_configured(macsec)) + return -EBUSY; + + ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, + macsec, &ctx); + if (!ops) + return -EOPNOTSUPP; + + macsec->offload = offload; + + ctx.secy = &macsec->secy; + ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops->mdo_del_secy, &ctx) : + macsec_offload(ops->mdo_add_secy, &ctx); + + if (ret) + macsec->offload = prev_offload; + + return ret; +} + static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) { struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1]; - enum macsec_offload offload, prev_offload; - int (*func)(struct macsec_context *ctx); struct nlattr **attrs = info->attrs; - struct net_device *dev; - const struct macsec_ops *ops; - struct macsec_context ctx; + enum macsec_offload offload; struct macsec_dev *macsec; + struct net_device *dev; int ret; if (!attrs[MACSEC_ATTR_IFINDEX]) @@ -2629,39 +2658,7 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) rtnl_lock(); - prev_offload = macsec->offload; - macsec->offload = offload; - - /* Check if the device already has rules configured: we do not support - * rules migration. - */ - if (macsec_is_configured(macsec)) { - ret = -EBUSY; - goto rollback; - } - - ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, - macsec, &ctx); - if (!ops) { - ret = -EOPNOTSUPP; - goto rollback; - } - - if (prev_offload == MACSEC_OFFLOAD_OFF) - func = ops->mdo_add_secy; - else - func = ops->mdo_del_secy; - - ctx.secy = &macsec->secy; - ret = macsec_offload(func, &ctx); - if (ret) - goto rollback; - - rtnl_unlock(); - return 0; - -rollback: - macsec->offload = prev_offload; + ret = macsec_update_offload(macsec, offload); rtnl_unlock(); return ret; @@ -3803,6 +3800,29 @@ static int macsec_changelink_common(struct net_device *dev, return 0; } +static int macsec_changelink_upd_offload(struct net_device *dev, struct nlattr *data[]) +{ + enum macsec_offload offload; + struct macsec_dev *macsec; + + macsec = macsec_priv(dev); + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); + + if (macsec->offload == offload) + return 0; + + /* Check if the offloading mode is supported by the underlying layers */ + if (offload != MACSEC_OFFLOAD_OFF && + !macsec_check_offload(offload, macsec)) + return -EOPNOTSUPP; + + /* Check if the net device is busy. */ + if (netif_running(dev)) + return -EBUSY; + + return macsec_update_offload(macsec, offload); +} + static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], struct nlattr *data[], struct netlink_ext_ack *extack) @@ -3831,6 +3851,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], if (ret) goto cleanup; + if (data[IFLA_MACSEC_OFFLOAD]) { + ret = macsec_changelink_upd_offload(dev, data); + if (ret) + goto cleanup; + } + /* If h/w offloading is available, propagate to the device */ if (macsec_is_offloaded(macsec)) { const struct macsec_ops *ops;