diff mbox series

[net-next,v1,01/10] xfrm: extend add policy callback to set failure reason

Message ID c182cae29914fa19ce970859e74234d3de506853.1674560845.git.leon@kernel.org (mailing list archive)
State Accepted
Commit 3089386db0901ac6ac3d99fbd601212c98217e60
Delegated to: Netdev Maintainers
Headers show
Series Convert drivers to return XFRM configuration errors through extack | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 4348 this patch: 4348
netdev/cc_maintainers warning 3 maintainers not CCed: linux-rdma@vger.kernel.org raeds@nvidia.com borisp@nvidia.com
netdev/build_clang success Errors and warnings before: 1020 this patch: 1020
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4558 this patch: 4558
netdev/checkpatch warning WARNING: Unnecessary space before function pointer arguments WARNING: line length of 94 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Leon Romanovsky Jan. 24, 2023, 11:54 a.m. UTC 
From: Leon Romanovsky <leonro@nvidia.com>

Almost all validation logic is in the drivers, but they are
missing reliable way to convey failure reason to userspace
applications.

Let's use extack to return this information to users.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 Documentation/networking/xfrm_device.rst                 | 2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c | 3 ++-
 include/linux/netdevice.h                                | 2 +-
 net/xfrm/xfrm_device.c                                   | 3 +--
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

Jakub Kicinski Jan. 25, 2023, 7:02 p.m. UTC | #1 
On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote:
> -	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
> +	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
>  	if (err) {
>  		xdo->dev = NULL;
>  		xdo->real_dev = NULL;
>  		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
>  		xdo->dir = 0;
>  		netdev_put(dev, &xdo->dev_tracker);
> -		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");

In a handful of places we do:

if (!extack->msg)
	NL_SET_ERR_MSG(extack, "Device failed to offload this policy");

in case the device did not provide the extack.
Dunno if it's worth doing here.
Leon Romanovsky Jan. 26, 2023, 7:28 a.m. UTC | #2 
On Wed, Jan 25, 2023 at 11:02:26AM -0800, Jakub Kicinski wrote:
> On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote:
> > -	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
> > +	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
> >  	if (err) {
> >  		xdo->dev = NULL;
> >  		xdo->real_dev = NULL;
> >  		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> >  		xdo->dir = 0;
> >  		netdev_put(dev, &xdo->dev_tracker);
> > -		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> 
> In a handful of places we do:
> 
> if (!extack->msg)
> 	NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> 
> in case the device did not provide the extack.
> Dunno if it's worth doing here.

Honestly, I followed devlink.c which didn't do that, but looked again
and found that devlink can potentially overwrite messages :)

For example in this case:
    997         err = ops->port_fn_state_get(port, &state, &opstate, extack);
    998         if (err) {
    999                 if (err == -EOPNOTSUPP)
   1000                         return 0;
   1001                 return err;
   1002         }
   1003         if (!devlink_port_fn_state_valid(state)) {
   1004                 WARN_ON_ONCE(1);
   1005                 NL_SET_ERR_MSG_MOD(extack, "Invalid state read from driver");
   1006                 return -EINVAL;
   1007         }


So what do you think about the following change, so we can leave
NL_SET_ERR_MSG_MOD() in devlink and xfrm intact? 

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 38f6334f408c..d6f3a958e30b 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -101,7 +101,7 @@ struct netlink_ext_ack {
                                                        \
        do_trace_netlink_extack(__msg);                 \
                                                        \
-       if (__extack)                                   \
+       if (__extack && !__extack->msg)                 \
                __extack->_msg = __msg;                 \
 } while (0)

@@ -111,7 +111,7 @@ struct netlink_ext_ack {
 #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {                         \
        struct netlink_ext_ack *__extack = (extack);                           \
                                                                               \
-       if (!__extack)                                                         \
+       if (!__extack || __extack->msg)                                        \
                break;                                                         \
        if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,               \
                     "%s" fmt "%s", "", ##args, "") >=                         \
Paolo Abeni Jan. 26, 2023, 9:45 a.m. UTC | #3 
On Thu, 2023-01-26 at 09:28 +0200, Leon Romanovsky wrote:
> On Wed, Jan 25, 2023 at 11:02:26AM -0800, Jakub Kicinski wrote:
> > On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote:
> > > -	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
> > > +	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
> > >  	if (err) {
> > >  		xdo->dev = NULL;
> > >  		xdo->real_dev = NULL;
> > >  		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> > >  		xdo->dir = 0;
> > >  		netdev_put(dev, &xdo->dev_tracker);
> > > -		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> > 
> > In a handful of places we do:
> > 
> > if (!extack->msg)
> > 	NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> > 
> > in case the device did not provide the extack.
> > Dunno if it's worth doing here.
> 
> Honestly, I followed devlink.c which didn't do that, but looked again
> and found that devlink can potentially overwrite messages :)
> 
> For example in this case:
>     997         err = ops->port_fn_state_get(port, &state, &opstate, extack);
>     998         if (err) {
>     999                 if (err == -EOPNOTSUPP)
>    1000                         return 0;
>    1001                 return err;
>    1002         }
>    1003         if (!devlink_port_fn_state_valid(state)) {
>    1004                 WARN_ON_ONCE(1);
>    1005                 NL_SET_ERR_MSG_MOD(extack, "Invalid state read from driver");
>    1006                 return -EINVAL;
>    1007         }
> 
> 
> So what do you think about the following change, so we can leave
> NL_SET_ERR_MSG_MOD() in devlink and xfrm intact? 
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 38f6334f408c..d6f3a958e30b 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -101,7 +101,7 @@ struct netlink_ext_ack {
>                                                         \
>         do_trace_netlink_extack(__msg);                 \
>                                                         \
> -       if (__extack)                                   \
> +       if (__extack && !__extack->msg)                 \
>                 __extack->_msg = __msg;                 \
>  } while (0)
> 
> @@ -111,7 +111,7 @@ struct netlink_ext_ack {
>  #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {                         \
>         struct netlink_ext_ack *__extack = (extack);                           \
>                                                                                \
> -       if (!__extack)                                                         \
> +       if (!__extack || __extack->msg)                                        \
>                 break;                                                         \
>         if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,               \
>                      "%s" fmt "%s", "", ##args, "") >=                         \
> 

I think it makes sense. With the above patch 3/10 should be updated to
preserve the 'catch-all' error message, I guess.

Let's see what Jakub thinks ;)

Cheers,

Paolo
Leon Romanovsky Jan. 26, 2023, 10:42 a.m. UTC | #4 
On Thu, Jan 26, 2023 at 10:45:50AM +0100, Paolo Abeni wrote:
> On Thu, 2023-01-26 at 09:28 +0200, Leon Romanovsky wrote:
> > On Wed, Jan 25, 2023 at 11:02:26AM -0800, Jakub Kicinski wrote:
> > > On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote:
> > > > -	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
> > > > +	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
> > > >  	if (err) {
> > > >  		xdo->dev = NULL;
> > > >  		xdo->real_dev = NULL;
> > > >  		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> > > >  		xdo->dir = 0;
> > > >  		netdev_put(dev, &xdo->dev_tracker);
> > > > -		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> > > 
> > > In a handful of places we do:
> > > 
> > > if (!extack->msg)
> > > 	NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> > > 
> > > in case the device did not provide the extack.
> > > Dunno if it's worth doing here.
> > 
> > Honestly, I followed devlink.c which didn't do that, but looked again
> > and found that devlink can potentially overwrite messages :)
> > 
> > For example in this case:
> >     997         err = ops->port_fn_state_get(port, &state, &opstate, extack);
> >     998         if (err) {
> >     999                 if (err == -EOPNOTSUPP)
> >    1000                         return 0;
> >    1001                 return err;
> >    1002         }
> >    1003         if (!devlink_port_fn_state_valid(state)) {
> >    1004                 WARN_ON_ONCE(1);
> >    1005                 NL_SET_ERR_MSG_MOD(extack, "Invalid state read from driver");
> >    1006                 return -EINVAL;
> >    1007         }
> > 
> > 
> > So what do you think about the following change, so we can leave
> > NL_SET_ERR_MSG_MOD() in devlink and xfrm intact? 
> > 
> > diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> > index 38f6334f408c..d6f3a958e30b 100644
> > --- a/include/linux/netlink.h
> > +++ b/include/linux/netlink.h
> > @@ -101,7 +101,7 @@ struct netlink_ext_ack {
> >                                                         \
> >         do_trace_netlink_extack(__msg);                 \
> >                                                         \
> > -       if (__extack)                                   \
> > +       if (__extack && !__extack->msg)                 \
> >                 __extack->_msg = __msg;                 \
> >  } while (0)
> > 
> > @@ -111,7 +111,7 @@ struct netlink_ext_ack {
> >  #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {                         \
> >         struct netlink_ext_ack *__extack = (extack);                           \
> >                                                                                \
> > -       if (!__extack)                                                         \
> > +       if (!__extack || __extack->msg)                                        \
> >                 break;                                                         \
> >         if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,               \
> >                      "%s" fmt "%s", "", ##args, "") >=                         \
> > 
> 
> I think it makes sense. With the above patch 3/10 should be updated to
> preserve the 'catch-all' error message, I guess.

Great, thanks

> 
> Let's see what Jakub thinks ;)
> 
> Cheers,
> 
> Paolo
>
Leon Romanovsky Jan. 26, 2023, 7:17 p.m. UTC | #5 
On Thu, Jan 26, 2023 at 12:42:08PM +0200, Leon Romanovsky wrote:
> On Thu, Jan 26, 2023 at 10:45:50AM +0100, Paolo Abeni wrote:
> > On Thu, 2023-01-26 at 09:28 +0200, Leon Romanovsky wrote:
> > > On Wed, Jan 25, 2023 at 11:02:26AM -0800, Jakub Kicinski wrote:
> > > > On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote:
> > > > > -	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
> > > > > +	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
> > > > >  	if (err) {
> > > > >  		xdo->dev = NULL;
> > > > >  		xdo->real_dev = NULL;
> > > > >  		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> > > > >  		xdo->dir = 0;
> > > > >  		netdev_put(dev, &xdo->dev_tracker);
> > > > > -		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> > > > 
> > > > In a handful of places we do:
> > > > 
> > > > if (!extack->msg)
> > > > 	NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
> > > > 
> > > > in case the device did not provide the extack.
> > > > Dunno if it's worth doing here.
> > > 
> > > Honestly, I followed devlink.c which didn't do that, but looked again
> > > and found that devlink can potentially overwrite messages :)
> > > 
> > > For example in this case:
> > >     997         err = ops->port_fn_state_get(port, &state, &opstate, extack);
> > >     998         if (err) {
> > >     999                 if (err == -EOPNOTSUPP)
> > >    1000                         return 0;
> > >    1001                 return err;
> > >    1002         }
> > >    1003         if (!devlink_port_fn_state_valid(state)) {
> > >    1004                 WARN_ON_ONCE(1);
> > >    1005                 NL_SET_ERR_MSG_MOD(extack, "Invalid state read from driver");
> > >    1006                 return -EINVAL;
> > >    1007         }
> > > 
> > > 
> > > So what do you think about the following change, so we can leave
> > > NL_SET_ERR_MSG_MOD() in devlink and xfrm intact? 
> > > 
> > > diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> > > index 38f6334f408c..d6f3a958e30b 100644
> > > --- a/include/linux/netlink.h
> > > +++ b/include/linux/netlink.h
> > > @@ -101,7 +101,7 @@ struct netlink_ext_ack {
> > >                                                         \
> > >         do_trace_netlink_extack(__msg);                 \
> > >                                                         \
> > > -       if (__extack)                                   \
> > > +       if (__extack && !__extack->msg)                 \
> > >                 __extack->_msg = __msg;                 \
> > >  } while (0)
> > > 
> > > @@ -111,7 +111,7 @@ struct netlink_ext_ack {
> > >  #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {                         \
> > >         struct netlink_ext_ack *__extack = (extack);                           \
> > >                                                                                \
> > > -       if (!__extack)                                                         \
> > > +       if (!__extack || __extack->msg)                                        \
> > >                 break;                                                         \
> > >         if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,               \
> > >                      "%s" fmt "%s", "", ##args, "") >=                         \
> > > 
> > 
> > I think it makes sense. With the above patch 3/10 should be updated to
> > preserve the 'catch-all' error message, I guess.
> 
> Great, thanks
> 
> > 
> > Let's see what Jakub thinks ;)

https://lore.kernel.org/netdev/2919eb55e2e9b92265a3ba600afc8137a901ae5f.1674760340.git.leon@kernel.org/T/#u

> > 
> > Cheers,
> > 
> > Paolo
> >
diff mbox series

Patch

diff --git a/Documentation/networking/xfrm_device.rst b/Documentation/networking/xfrm_device.rst
index c43ace79e320..b9c53e626982 100644
--- a/Documentation/networking/xfrm_device.rst
+++ b/Documentation/networking/xfrm_device.rst
@@ -73,7 +73,7 @@  Callbacks to implement
 
         /* Solely packet offload callbacks */
 	void    (*xdo_dev_state_update_curlft) (struct xfrm_state *x);
-	int	(*xdo_dev_policy_add) (struct xfrm_policy *x);
+	int	(*xdo_dev_policy_add) (struct xfrm_policy *x, struct netlink_ext_ack *extack);
 	void	(*xdo_dev_policy_delete) (struct xfrm_policy *x);
 	void	(*xdo_dev_policy_free) (struct xfrm_policy *x);
   };
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
index bb9023957f74..83e0f874484e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -550,7 +550,8 @@  mlx5e_ipsec_build_accel_pol_attrs(struct mlx5e_ipsec_pol_entry *pol_entry,
 	attrs->reqid = x->xfrm_vec[0].reqid;
 }
 
-static int mlx5e_xfrm_add_policy(struct xfrm_policy *x)
+static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
+				 struct netlink_ext_ack *extack)
 {
 	struct net_device *netdev = x->xdo.real_dev;
 	struct mlx5e_ipsec_pol_entry *pol_entry;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index aad12a179e54..7c43b9fb9aae 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1042,7 +1042,7 @@  struct xfrmdev_ops {
 				       struct xfrm_state *x);
 	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
 	void	(*xdo_dev_state_update_curlft) (struct xfrm_state *x);
-	int	(*xdo_dev_policy_add) (struct xfrm_policy *x);
+	int	(*xdo_dev_policy_add) (struct xfrm_policy *x, struct netlink_ext_ack *extack);
 	void	(*xdo_dev_policy_delete) (struct xfrm_policy *x);
 	void	(*xdo_dev_policy_free) (struct xfrm_policy *x);
 };
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 4aff76c6f12e..2cec637a4a9c 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -383,14 +383,13 @@  int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
 		return -EINVAL;
 	}
 
-	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
+	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
 	if (err) {
 		xdo->dev = NULL;
 		xdo->real_dev = NULL;
 		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
 		xdo->dir = 0;
 		netdev_put(dev, &xdo->dev_tracker);
-		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
 		return err;
 	}