* [PATCH net-next v2 01/10] genetlink: push conditional locking into dumpit/done
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 02/10] genetlink: make genl_info->nlhdr const Jakub Kicinski
` (8 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Jiri Pirko, jacob.e.keller
Add helpers which take/release the genl mutex based
on family->parallel_ops. Remove the separation between
handling of ops in locked and parallel families.
Future patches would make the duplicated code grow even more.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
CC: jacob.e.keller@intel.com
---
net/netlink/genetlink.c | 90 ++++++++++++++++-------------------------
1 file changed, 35 insertions(+), 55 deletions(-)
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 6bd2ce51271f..0d4285688ab9 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -52,6 +52,18 @@ static void genl_unlock_all(void)
up_write(&cb_lock);
}
+static void genl_op_lock(const struct genl_family *family)
+{
+ if (!family->parallel_ops)
+ genl_lock();
+}
+
+static void genl_op_unlock(const struct genl_family *family)
+{
+ if (!family->parallel_ops)
+ genl_unlock();
+}
+
static DEFINE_IDR(genl_fam_idr);
/*
@@ -838,11 +850,9 @@ static int genl_start(struct netlink_callback *cb)
cb->data = info;
if (ops->start) {
- if (!ctx->family->parallel_ops)
- genl_lock();
+ genl_op_lock(ctx->family);
rc = ops->start(cb);
- if (!ctx->family->parallel_ops)
- genl_unlock();
+ genl_op_unlock(ctx->family);
}
if (rc) {
@@ -853,46 +863,34 @@ static int genl_start(struct netlink_callback *cb)
return rc;
}
-static int genl_lock_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+static int genl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
{
- const struct genl_split_ops *ops = &genl_dumpit_info(cb)->op;
+ const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+ const struct genl_split_ops *ops = &info->op;
int rc;
- genl_lock();
+ genl_op_lock(info->family);
rc = ops->dumpit(skb, cb);
- genl_unlock();
+ genl_op_unlock(info->family);
return rc;
}
-static int genl_lock_done(struct netlink_callback *cb)
+static int genl_done(struct netlink_callback *cb)
{
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
const struct genl_split_ops *ops = &info->op;
int rc = 0;
if (ops->done) {
- genl_lock();
+ genl_op_lock(info->family);
rc = ops->done(cb);
- genl_unlock();
+ genl_op_unlock(info->family);
}
genl_family_rcv_msg_attrs_free(info->attrs);
genl_dumpit_info_free(info);
return rc;
}
-static int genl_parallel_done(struct netlink_callback *cb)
-{
- const struct genl_dumpit_info *info = genl_dumpit_info(cb);
- const struct genl_split_ops *ops = &info->op;
- int rc = 0;
-
- if (ops->done)
- rc = ops->done(cb);
- genl_family_rcv_msg_attrs_free(info->attrs);
- genl_dumpit_info_free(info);
- return rc;
-}
-
static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
struct sk_buff *skb,
struct nlmsghdr *nlh,
@@ -901,6 +899,14 @@ static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
int hdrlen, struct net *net)
{
struct genl_start_context ctx;
+ struct netlink_dump_control c = {
+ .module = family->module,
+ .data = &ctx,
+ .start = genl_start,
+ .dump = genl_dumpit,
+ .done = genl_done,
+ .extack = extack,
+ };
int err;
ctx.family = family;
@@ -909,31 +915,9 @@ static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
ctx.ops = ops;
ctx.hdrlen = hdrlen;
- if (!family->parallel_ops) {
- struct netlink_dump_control c = {
- .module = family->module,
- .data = &ctx,
- .start = genl_start,
- .dump = genl_lock_dumpit,
- .done = genl_lock_done,
- .extack = extack,
- };
-
- genl_unlock();
- err = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
- genl_lock();
- } else {
- struct netlink_dump_control c = {
- .module = family->module,
- .data = &ctx,
- .start = genl_start,
- .dump = ops->dumpit,
- .done = genl_parallel_done,
- .extack = extack,
- };
-
- err = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
- }
+ genl_op_unlock(family);
+ err = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
+ genl_op_lock(family);
return err;
}
@@ -1065,13 +1049,9 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
if (family == NULL)
return -ENOENT;
- if (!family->parallel_ops)
- genl_lock();
-
+ genl_op_lock(family);
err = genl_family_rcv_msg(family, skb, nlh, extack);
-
- if (!family->parallel_ops)
- genl_unlock();
+ genl_op_unlock(family);
return err;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 02/10] genetlink: make genl_info->nlhdr const
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 01/10] genetlink: push conditional locking into dumpit/done Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 03/10] genetlink: remove userhdr from struct genl_info Jakub Kicinski
` (7 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Jiri Pirko, sam
struct netlink_callback has a const nlh pointer, make the
pointer in struct genl_info const as well, to make copying
between the two easier.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
CC: sam@mendozajonas.com
---
include/net/genetlink.h | 2 +-
net/ncsi/ncsi-netlink.c | 2 +-
net/ncsi/ncsi-netlink.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index ed4622dd4828..0366d0925596 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -104,7 +104,7 @@ struct genl_family {
struct genl_info {
u32 snd_seq;
u32 snd_portid;
- struct nlmsghdr * nlhdr;
+ const struct nlmsghdr * nlhdr;
struct genlmsghdr * genlhdr;
void * userhdr;
struct nlattr ** attrs;
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index d27f4eccce6d..a3a6753a1db7 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -563,7 +563,7 @@ int ncsi_send_netlink_timeout(struct ncsi_request *nr,
int ncsi_send_netlink_err(struct net_device *dev,
u32 snd_seq,
u32 snd_portid,
- struct nlmsghdr *nlhdr,
+ const struct nlmsghdr *nlhdr,
int err)
{
struct nlmsghdr *nlh;
diff --git a/net/ncsi/ncsi-netlink.h b/net/ncsi/ncsi-netlink.h
index 39a1a9d7bf77..747767ea0aae 100644
--- a/net/ncsi/ncsi-netlink.h
+++ b/net/ncsi/ncsi-netlink.h
@@ -19,7 +19,7 @@ int ncsi_send_netlink_timeout(struct ncsi_request *nr,
int ncsi_send_netlink_err(struct net_device *dev,
u32 snd_seq,
u32 snd_portid,
- struct nlmsghdr *nlhdr,
+ const struct nlmsghdr *nlhdr,
int err);
#endif /* __NCSI_NETLINK_H__ */
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 03/10] genetlink: remove userhdr from struct genl_info
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 01/10] genetlink: push conditional locking into dumpit/done Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 02/10] genetlink: make genl_info->nlhdr const Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 04/10] genetlink: add struct genl_info to struct genl_dumpit_info Jakub Kicinski
` (6 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Jiri Pirko, philipp.reisner, lars.ellenberg,
christoph.boehmwalder, axboe, pshelar, jmaloy, ying.xue,
jacob.e.keller, drbd-dev, linux-block, dev, tipc-discussion
Only three families use info->userhdr today and going forward
we discourage using fixed headers in new families.
So having the pointer to user header in struct genl_info
is an overkill. Compute the header pointer at runtime.
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: philipp.reisner@linbit.com
CC: lars.ellenberg@linbit.com
CC: christoph.boehmwalder@linbit.com
CC: axboe@kernel.dk
CC: pshelar@ovn.org
CC: jmaloy@redhat.com
CC: ying.xue@windriver.com
CC: jacob.e.keller@intel.com
CC: drbd-dev@lists.linbit.com
CC: linux-block@vger.kernel.org
CC: dev@openvswitch.org
CC: tipc-discussion@lists.sourceforge.net
---
drivers/block/drbd/drbd_nl.c | 9 +++++----
include/net/genetlink.h | 7 +++++--
net/netlink/genetlink.c | 1 -
net/openvswitch/conntrack.c | 2 +-
net/openvswitch/datapath.c | 29 ++++++++++++++++-------------
net/openvswitch/meter.c | 10 +++++-----
net/tipc/netlink_compat.c | 2 +-
7 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index cddae6f4b00f..d3538bd83fb3 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -159,7 +159,7 @@ static int drbd_msg_sprintf_info(struct sk_buff *skb, const char *fmt, ...)
static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
struct sk_buff *skb, struct genl_info *info, unsigned flags)
{
- struct drbd_genlmsghdr *d_in = info->userhdr;
+ struct drbd_genlmsghdr *d_in = genl_info_userhdr(info);
const u8 cmd = info->genlhdr->cmd;
int err;
@@ -1396,8 +1396,9 @@ static void drbd_suspend_al(struct drbd_device *device)
static bool should_set_defaults(struct genl_info *info)
{
- unsigned flags = ((struct drbd_genlmsghdr*)info->userhdr)->flags;
- return 0 != (flags & DRBD_GENL_F_SET_DEFAULTS);
+ struct drbd_genlmsghdr *dh = genl_info_userhdr(info);
+
+ return 0 != (dh->flags & DRBD_GENL_F_SET_DEFAULTS);
}
static unsigned int drbd_al_extents_max(struct drbd_backing_dev *bdev)
@@ -4276,7 +4277,7 @@ static void device_to_info(struct device_info *info,
int drbd_adm_new_minor(struct sk_buff *skb, struct genl_info *info)
{
struct drbd_config_context adm_ctx;
- struct drbd_genlmsghdr *dh = info->userhdr;
+ struct drbd_genlmsghdr *dh = genl_info_userhdr(info);
enum drbd_ret_code retcode;
retcode = drbd_adm_prepare(&adm_ctx, skb, info, DRBD_ADM_NEED_RESOURCE);
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 0366d0925596..9dc21ec15734 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -95,7 +95,6 @@ struct genl_family {
* @snd_portid: netlink portid of sender
* @nlhdr: netlink message header
* @genlhdr: generic netlink message header
- * @userhdr: user specific header
* @attrs: netlink attributes
* @_net: network namespace
* @user_ptr: user pointers
@@ -106,7 +105,6 @@ struct genl_info {
u32 snd_portid;
const struct nlmsghdr * nlhdr;
struct genlmsghdr * genlhdr;
- void * userhdr;
struct nlattr ** attrs;
possible_net_t _net;
void * user_ptr[2];
@@ -123,6 +121,11 @@ static inline void genl_info_net_set(struct genl_info *info, struct net *net)
write_pnet(&info->_net, net);
}
+static inline void *genl_info_userhdr(const struct genl_info *info)
+{
+ return (u8 *)info->genlhdr + GENL_HDRLEN;
+}
+
#define GENL_SET_ERR_MSG(info, msg) NL_SET_ERR_MSG((info)->extack, msg)
#define GENL_SET_ERR_MSG_FMT(info, msg, args...) \
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0d4285688ab9..f98f730bb245 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -943,7 +943,6 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
info.snd_portid = NETLINK_CB(skb).portid;
info.nlhdr = nlh;
info.genlhdr = nlmsg_data(nlh);
- info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
info.attrs = attrbuf;
info.extack = extack;
genl_info_net_set(&info, net);
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index fa955e892210..03a6220b7d99 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1604,7 +1604,7 @@ static struct sk_buff *
ovs_ct_limit_cmd_reply_start(struct genl_info *info, u8 cmd,
struct ovs_header **ovs_reply_header)
{
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct sk_buff *skb;
skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a6d2a0b1aa21..80f5f755b0e6 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -589,7 +589,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
{
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct net *net = sock_net(skb->sk);
struct nlattr **a = info->attrs;
struct sw_flow_actions *acts;
@@ -966,7 +966,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
{
struct net *net = sock_net(skb->sk);
struct nlattr **a = info->attrs;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct sw_flow *flow = NULL, *new_flow;
struct sw_flow_mask mask;
struct sk_buff *reply;
@@ -1213,7 +1213,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
{
struct net *net = sock_net(skb->sk);
struct nlattr **a = info->attrs;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct sw_flow_key key;
struct sw_flow *flow;
struct sk_buff *reply = NULL;
@@ -1314,7 +1314,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr **a = info->attrs;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct net *net = sock_net(skb->sk);
struct sw_flow_key key;
struct sk_buff *reply;
@@ -1373,7 +1373,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr **a = info->attrs;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct net *net = sock_net(skb->sk);
struct sw_flow_key key;
struct sk_buff *reply;
@@ -1641,7 +1641,7 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb,
{
struct datapath *dp;
- dp = lookup_datapath(sock_net(skb->sk), info->userhdr,
+ dp = lookup_datapath(sock_net(skb->sk), genl_info_userhdr(info),
info->attrs);
if (IS_ERR(dp))
return;
@@ -1934,7 +1934,8 @@ static int ovs_dp_cmd_del(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
ovs_lock();
- dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
+ dp = lookup_datapath(sock_net(skb->sk), genl_info_userhdr(info),
+ info->attrs);
err = PTR_ERR(dp);
if (IS_ERR(dp))
goto err_unlock_free;
@@ -1967,7 +1968,8 @@ static int ovs_dp_cmd_set(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
ovs_lock();
- dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
+ dp = lookup_datapath(sock_net(skb->sk), genl_info_userhdr(info),
+ info->attrs);
err = PTR_ERR(dp);
if (IS_ERR(dp))
goto err_unlock_free;
@@ -2002,7 +2004,8 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
ovs_lock();
- dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
+ dp = lookup_datapath(sock_net(skb->sk), genl_info_userhdr(info),
+ info->attrs);
if (IS_ERR(dp)) {
err = PTR_ERR(dp);
goto err_unlock_free;
@@ -2245,7 +2248,7 @@ static void ovs_update_headroom(struct datapath *dp, unsigned int new_headroom)
static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr **a = info->attrs;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct vport_parms parms;
struct sk_buff *reply;
struct vport *vport;
@@ -2347,7 +2350,7 @@ static int ovs_vport_cmd_set(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
ovs_lock();
- vport = lookup_vport(sock_net(skb->sk), info->userhdr, a);
+ vport = lookup_vport(sock_net(skb->sk), genl_info_userhdr(info), a);
err = PTR_ERR(vport);
if (IS_ERR(vport))
goto exit_unlock_free;
@@ -2403,7 +2406,7 @@ static int ovs_vport_cmd_del(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
ovs_lock();
- vport = lookup_vport(sock_net(skb->sk), info->userhdr, a);
+ vport = lookup_vport(sock_net(skb->sk), genl_info_userhdr(info), a);
err = PTR_ERR(vport);
if (IS_ERR(vport))
goto exit_unlock_free;
@@ -2446,7 +2449,7 @@ static int ovs_vport_cmd_del(struct sk_buff *skb, struct genl_info *info)
static int ovs_vport_cmd_get(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr **a = info->attrs;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct sk_buff *reply;
struct vport *vport;
int err;
diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
index c4ebf810e4b1..cc08e0403909 100644
--- a/net/openvswitch/meter.c
+++ b/net/openvswitch/meter.c
@@ -211,7 +211,7 @@ ovs_meter_cmd_reply_start(struct genl_info *info, u8 cmd,
struct ovs_header **ovs_reply_header)
{
struct sk_buff *skb;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
if (!skb)
@@ -272,7 +272,7 @@ static int ovs_meter_cmd_reply_stats(struct sk_buff *reply, u32 meter_id,
static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info)
{
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct ovs_header *ovs_reply_header;
struct nlattr *nla, *band_nla;
struct sk_buff *reply;
@@ -409,7 +409,7 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
struct dp_meter *meter, *old_meter;
struct sk_buff *reply;
struct ovs_header *ovs_reply_header;
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct dp_meter_table *meter_tbl;
struct datapath *dp;
int err;
@@ -482,7 +482,7 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
static int ovs_meter_cmd_get(struct sk_buff *skb, struct genl_info *info)
{
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct ovs_header *ovs_reply_header;
struct nlattr **a = info->attrs;
struct dp_meter *meter;
@@ -535,7 +535,7 @@ static int ovs_meter_cmd_get(struct sk_buff *skb, struct genl_info *info)
static int ovs_meter_cmd_del(struct sk_buff *skb, struct genl_info *info)
{
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct ovs_header *ovs_reply_header;
struct nlattr **a = info->attrs;
struct dp_meter *old_meter;
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 9b47c8409231..299cd6754f14 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -1294,7 +1294,7 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, struct genl_info *info)
struct tipc_nl_compat_msg msg;
struct nlmsghdr *req_nlh;
struct nlmsghdr *rep_nlh;
- struct tipc_genlmsghdr *req_userhdr = info->userhdr;
+ struct tipc_genlmsghdr *req_userhdr = genl_info_userhdr(info);
memset(&msg, 0, sizeof(msg));
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 04/10] genetlink: add struct genl_info to struct genl_dumpit_info
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (2 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 03/10] genetlink: remove userhdr from struct genl_info Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 05/10] genetlink: use attrs from struct genl_info Jakub Kicinski
` (5 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Jiri Pirko
Netlink GET implementations must currently juggle struct genl_info
and struct netlink_callback, depending on whether they were called
from doit or dumpit.
Add genl_info to the dump state and populate the fields.
This way implementations can simply pass struct genl_info around.
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/genetlink.h | 8 ++++++++
net/netlink/genetlink.c | 16 ++++++++++++++--
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 9dc21ec15734..86c8eaaa3a43 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -250,11 +250,13 @@ struct genl_split_ops {
* @family: generic netlink family - for internal genl code usage
* @op: generic netlink ops - for internal genl code usage
* @attrs: netlink attributes
+ * @info: struct genl_info describing the request
*/
struct genl_dumpit_info {
const struct genl_family *family;
struct genl_split_ops op;
struct nlattr **attrs;
+ struct genl_info info;
};
static inline const struct genl_dumpit_info *
@@ -263,6 +265,12 @@ genl_dumpit_info(struct netlink_callback *cb)
return cb->data;
}
+static inline const struct genl_info *
+genl_info_dump(struct netlink_callback *cb)
+{
+ return &genl_dumpit_info(cb)->info;
+}
+
int genl_register_family(struct genl_family *family);
int genl_unregister_family(const struct genl_family *family);
void genl_notify(const struct genl_family *family, struct sk_buff *skb,
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index f98f730bb245..82ad26970b9b 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -847,6 +847,14 @@ static int genl_start(struct netlink_callback *cb)
info->family = ctx->family;
info->op = *ops;
info->attrs = attrs;
+ info->info.snd_seq = cb->nlh->nlmsg_seq;
+ info->info.snd_portid = NETLINK_CB(cb->skb).portid;
+ info->info.nlhdr = cb->nlh;
+ info->info.genlhdr = nlmsg_data(cb->nlh);
+ info->info.attrs = attrs;
+ genl_info_net_set(&info->info, sock_net(cb->skb->sk));
+ info->info.extack = cb->extack;
+ memset(&info->info.user_ptr, 0, sizeof(info->info.user_ptr));
cb->data = info;
if (ops->start) {
@@ -865,10 +873,12 @@ static int genl_start(struct netlink_callback *cb)
static int genl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
{
- const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+ struct genl_dumpit_info *info = cb->data;
const struct genl_split_ops *ops = &info->op;
int rc;
+ info->info.extack = cb->extack;
+
genl_op_lock(info->family);
rc = ops->dumpit(skb, cb);
genl_op_unlock(info->family);
@@ -877,10 +887,12 @@ static int genl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
static int genl_done(struct netlink_callback *cb)
{
- const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+ struct genl_dumpit_info *info = cb->data;
const struct genl_split_ops *ops = &info->op;
int rc = 0;
+ info->info.extack = cb->extack;
+
if (ops->done) {
genl_op_lock(info->family);
rc = ops->done(cb);
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 05/10] genetlink: use attrs from struct genl_info
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (3 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 04/10] genetlink: add struct genl_info to struct genl_dumpit_info Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 06/10] genetlink: add a family pointer to " Jakub Kicinski
` (4 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Miquel Raynal, Jiri Pirko, Jason, alex.aring, stefan,
krzysztof.kozlowski, jmaloy, ying.xue, floridsleeves, leon,
jacob.e.keller, wireguard, linux-wpan, tipc-discussion
Since dumps carry struct genl_info now, use the attrs pointer
from genl_info and remove the one in struct genl_dumpit_info.
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: Jason@zx2c4.com
CC: jiri@resnulli.us
CC: alex.aring@gmail.com
CC: stefan@datenfreihafen.org
CC: krzysztof.kozlowski@linaro.org
CC: jmaloy@redhat.com
CC: ying.xue@windriver.com
CC: floridsleeves@gmail.com
CC: leon@kernel.org
CC: jacob.e.keller@intel.com
CC: wireguard@lists.zx2c4.com
CC: linux-wpan@vger.kernel.org
CC: tipc-discussion@lists.sourceforge.net
---
drivers/net/wireguard/netlink.c | 2 +-
include/net/genetlink.h | 1 -
net/devlink/health.c | 2 +-
net/devlink/leftover.c | 6 +++---
net/ethtool/netlink.c | 3 ++-
net/ethtool/tunnels.c | 2 +-
net/ieee802154/nl802154.c | 4 ++--
net/netlink/genetlink.c | 7 +++----
net/nfc/netlink.c | 4 ++--
net/tipc/netlink_compat.c | 2 +-
net/tipc/node.c | 4 ++--
net/tipc/socket.c | 2 +-
net/tipc/udp_media.c | 2 +-
13 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index 6d1bd9f52d02..dc09b75a3248 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -200,7 +200,7 @@ static int wg_get_device_start(struct netlink_callback *cb)
{
struct wg_device *wg;
- wg = lookup_interface(genl_dumpit_info(cb)->attrs, cb->skb);
+ wg = lookup_interface(genl_info_dump(cb)->attrs, cb->skb);
if (IS_ERR(wg))
return PTR_ERR(wg);
DUMP_CTX(cb)->wg = wg;
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 86c8eaaa3a43..a8a15b9c22c8 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -255,7 +255,6 @@ struct genl_split_ops {
struct genl_dumpit_info {
const struct genl_family *family;
struct genl_split_ops op;
- struct nlattr **attrs;
struct genl_info info;
};
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 194340a8bb86..b8b3c09eea9e 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -1250,7 +1250,7 @@ devlink_health_reporter_get_from_cb(struct netlink_callback *cb)
{
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct devlink_health_reporter *reporter;
- struct nlattr **attrs = info->attrs;
+ struct nlattr **attrs = info->info.attrs;
struct devlink *devlink;
devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index e7900d9fa205..39538fb61008 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -5172,7 +5172,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
struct devlink_nl_dump_state *state = devlink_dump_state(cb);
struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
u64 ret_offset, start_offset, end_offset = U64_MAX;
- struct nlattr **attrs = info->attrs;
+ struct nlattr **attrs = info->info.attrs;
struct devlink_port *port = NULL;
devlink_chunk_fill_t *region_cb;
struct devlink_region *region;
@@ -5195,8 +5195,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
goto out_unlock;
}
- if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
- index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+ if (attrs[DEVLINK_ATTR_PORT_INDEX]) {
+ index = nla_get_u32(attrs[DEVLINK_ATTR_PORT_INDEX]);
port = devlink_port_get_by_index(devlink, index);
if (!port) {
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index ae344f1b0bbd..9fc7c41f4786 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -538,7 +538,8 @@ static int ethnl_default_start(struct netlink_callback *cb)
goto free_req_info;
}
- ret = ethnl_default_parse(req_info, info->attrs, sock_net(cb->skb->sk),
+ ret = ethnl_default_parse(req_info, info->info.attrs,
+ sock_net(cb->skb->sk),
ops, cb->extack, false);
if (req_info->dev) {
/* We ignore device specification in dump requests but as the
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index 05f752557b5e..b4ce47dd2aa6 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -219,7 +219,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb)
{
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ethnl_tunnel_info_dump_ctx *ctx = (void *)cb->ctx;
- struct nlattr **tb = info->attrs;
+ struct nlattr **tb = info->info.attrs;
int ret;
BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index d610c1886160..1a265a421308 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -262,7 +262,7 @@ nl802154_prepare_wpan_dev_dump(struct sk_buff *skb,
if (!cb->args[0]) {
*wpan_dev = __cfg802154_wpan_dev_from_attrs(sock_net(skb->sk),
- info->attrs);
+ info->info.attrs);
if (IS_ERR(*wpan_dev)) {
err = PTR_ERR(*wpan_dev);
goto out_unlock;
@@ -570,7 +570,7 @@ static int nl802154_dump_wpan_phy_parse(struct sk_buff *skb,
struct nl802154_dump_wpan_phy_state *state)
{
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
- struct nlattr **tb = info->attrs;
+ struct nlattr **tb = info->info.attrs;
if (tb[NL802154_ATTR_WPAN_PHY])
state->filter_wpan_phy = nla_get_u32(tb[NL802154_ATTR_WPAN_PHY]);
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 82ad26970b9b..d47879d5a74c 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -846,7 +846,6 @@ static int genl_start(struct netlink_callback *cb)
}
info->family = ctx->family;
info->op = *ops;
- info->attrs = attrs;
info->info.snd_seq = cb->nlh->nlmsg_seq;
info->info.snd_portid = NETLINK_CB(cb->skb).portid;
info->info.nlhdr = cb->nlh;
@@ -864,7 +863,7 @@ static int genl_start(struct netlink_callback *cb)
}
if (rc) {
- genl_family_rcv_msg_attrs_free(info->attrs);
+ genl_family_rcv_msg_attrs_free(info->info.attrs);
genl_dumpit_info_free(info);
cb->data = NULL;
}
@@ -898,7 +897,7 @@ static int genl_done(struct netlink_callback *cb)
rc = ops->done(cb);
genl_op_unlock(info->family);
}
- genl_family_rcv_msg_attrs_free(info->attrs);
+ genl_family_rcv_msg_attrs_free(info->info.attrs);
genl_dumpit_info_free(info);
return rc;
}
@@ -1387,7 +1386,7 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
{
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
- struct nlattr **tb = info->attrs;
+ struct nlattr **tb = info->info.attrs;
const struct genl_family *rt;
struct genl_op_iter i;
int err;
diff --git a/net/nfc/netlink.c b/net/nfc/netlink.c
index e9ac6a6f934e..aa1dbf654c3e 100644
--- a/net/nfc/netlink.c
+++ b/net/nfc/netlink.c
@@ -110,10 +110,10 @@ static struct nfc_dev *__get_device_from_cb(struct netlink_callback *cb)
struct nfc_dev *dev;
u32 idx;
- if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
+ if (!info->info.attrs[NFC_ATTR_DEVICE_INDEX])
return ERR_PTR(-EINVAL);
- idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
+ idx = nla_get_u32(info->info.attrs[NFC_ATTR_DEVICE_INDEX]);
dev = nfc_get_device(idx);
if (!dev)
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 299cd6754f14..5bc076f2fa74 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -208,7 +208,7 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
goto err_out;
}
- info.attrs = attrbuf;
+ info.info.attrs = attrbuf;
if (nlmsg_len(cb.nlh) > 0) {
err = nlmsg_parse_deprecated(cb.nlh, GENL_HDRLEN, attrbuf,
diff --git a/net/tipc/node.c b/net/tipc/node.c
index a9c5b6594889..3105abe97bb9 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -2662,7 +2662,7 @@ static int __tipc_nl_add_node_links(struct net *net, struct tipc_nl_msg *msg,
int tipc_nl_node_dump_link(struct sk_buff *skb, struct netlink_callback *cb)
{
struct net *net = sock_net(skb->sk);
- struct nlattr **attrs = genl_dumpit_info(cb)->attrs;
+ struct nlattr **attrs = genl_dumpit_info(cb)->info.attrs;
struct nlattr *link[TIPC_NLA_LINK_MAX + 1];
struct tipc_net *tn = net_generic(net, tipc_net_id);
struct tipc_node *node;
@@ -2870,7 +2870,7 @@ int tipc_nl_node_dump_monitor_peer(struct sk_buff *skb,
int err;
if (!prev_node) {
- struct nlattr **attrs = genl_dumpit_info(cb)->attrs;
+ struct nlattr **attrs = genl_dumpit_info(cb)->info.attrs;
struct nlattr *mon[TIPC_NLA_MON_MAX + 1];
if (!attrs[TIPC_NLA_MON])
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index ef8e5139a873..bb1118d02f95 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -3791,7 +3791,7 @@ int tipc_nl_publ_dump(struct sk_buff *skb, struct netlink_callback *cb)
struct tipc_sock *tsk;
if (!tsk_portid) {
- struct nlattr **attrs = genl_dumpit_info(cb)->attrs;
+ struct nlattr **attrs = genl_dumpit_info(cb)->info.attrs;
struct nlattr *sock[TIPC_NLA_SOCK_MAX + 1];
if (!attrs[TIPC_NLA_SOCK])
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 926232557e77..f892b0903dba 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -465,7 +465,7 @@ int tipc_udp_nl_dump_remoteip(struct sk_buff *skb, struct netlink_callback *cb)
int i;
if (!bid && !skip_cnt) {
- struct nlattr **attrs = genl_dumpit_info(cb)->attrs;
+ struct nlattr **attrs = genl_dumpit_info(cb)->info.attrs;
struct net *net = sock_net(skb->sk);
struct nlattr *battrs[TIPC_NLA_BEARER_MAX + 1];
char *bname;
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 06/10] genetlink: add a family pointer to struct genl_info
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (4 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 05/10] genetlink: use attrs from struct genl_info Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-11 6:28 ` Jiri Pirko
2023-08-10 23:38 ` [PATCH net-next v2 07/10] genetlink: add genlmsg_iput() API Jakub Kicinski
` (3 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski
Having family in struct genl_info is quite useful. It cuts
down the number of arguments which need to be passed to
helpers which already take struct genl_info.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/genetlink.h | 4 ++--
net/netlink/genetlink.c | 21 ++++++++++++---------
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index a8a15b9c22c8..6b858c4cba5b 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -93,6 +93,7 @@ struct genl_family {
* struct genl_info - receiving information
* @snd_seq: sending sequence number
* @snd_portid: netlink portid of sender
+ * @family: generic netlink family
* @nlhdr: netlink message header
* @genlhdr: generic netlink message header
* @attrs: netlink attributes
@@ -103,6 +104,7 @@ struct genl_family {
struct genl_info {
u32 snd_seq;
u32 snd_portid;
+ const struct genl_family *family;
const struct nlmsghdr * nlhdr;
struct genlmsghdr * genlhdr;
struct nlattr ** attrs;
@@ -247,13 +249,11 @@ struct genl_split_ops {
/**
* struct genl_dumpit_info - info that is available during dumpit op call
- * @family: generic netlink family - for internal genl code usage
* @op: generic netlink ops - for internal genl code usage
* @attrs: netlink attributes
* @info: struct genl_info describing the request
*/
struct genl_dumpit_info {
- const struct genl_family *family;
struct genl_split_ops op;
struct genl_info info;
};
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index d47879d5a74c..8315d31b53db 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -844,8 +844,8 @@ static int genl_start(struct netlink_callback *cb)
genl_family_rcv_msg_attrs_free(attrs);
return -ENOMEM;
}
- info->family = ctx->family;
info->op = *ops;
+ info->info.family = ctx->family;
info->info.snd_seq = cb->nlh->nlmsg_seq;
info->info.snd_portid = NETLINK_CB(cb->skb).portid;
info->info.nlhdr = cb->nlh;
@@ -872,11 +872,12 @@ static int genl_start(struct netlink_callback *cb)
static int genl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
{
- struct genl_dumpit_info *info = cb->data;
- const struct genl_split_ops *ops = &info->op;
+ struct genl_dumpit_info *dump_info = cb->data;
+ const struct genl_split_ops *ops = &dump_info->op;
+ struct genl_info *info = &dump_info->info;
int rc;
- info->info.extack = cb->extack;
+ info->extack = cb->extack;
genl_op_lock(info->family);
rc = ops->dumpit(skb, cb);
@@ -886,19 +887,20 @@ static int genl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
static int genl_done(struct netlink_callback *cb)
{
- struct genl_dumpit_info *info = cb->data;
- const struct genl_split_ops *ops = &info->op;
+ struct genl_dumpit_info *dump_info = cb->data;
+ const struct genl_split_ops *ops = &dump_info->op;
+ struct genl_info *info = &dump_info->info;
int rc = 0;
- info->info.extack = cb->extack;
+ info->extack = cb->extack;
if (ops->done) {
genl_op_lock(info->family);
rc = ops->done(cb);
genl_op_unlock(info->family);
}
- genl_family_rcv_msg_attrs_free(info->info.attrs);
- genl_dumpit_info_free(info);
+ genl_family_rcv_msg_attrs_free(info->attrs);
+ genl_dumpit_info_free(dump_info);
return rc;
}
@@ -952,6 +954,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
info.snd_seq = nlh->nlmsg_seq;
info.snd_portid = NETLINK_CB(skb).portid;
+ info.family = family;
info.nlhdr = nlh;
info.genlhdr = nlmsg_data(nlh);
info.attrs = attrbuf;
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 07/10] genetlink: add genlmsg_iput() API
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (5 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 06/10] genetlink: add a family pointer to " Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-11 6:44 ` Jiri Pirko
2023-08-10 23:38 ` [PATCH net-next v2 08/10] netdev-genl: use struct genl_info for reply construction Jakub Kicinski
` (2 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski
Add some APIs and helpers required for convenient construction
of replies and notifications based on struct genl_info.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/genetlink.h | 54 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 6b858c4cba5b..e18a4c0d69ee 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -113,7 +113,7 @@ struct genl_info {
struct netlink_ext_ack *extack;
};
-static inline struct net *genl_info_net(struct genl_info *info)
+static inline struct net *genl_info_net(const struct genl_info *info)
{
return read_pnet(&info->_net);
}
@@ -270,6 +270,32 @@ genl_info_dump(struct netlink_callback *cb)
return &genl_dumpit_info(cb)->info;
}
+/**
+ * genl_info_init_ntf() - initialize genl_info for notifications
+ * @info: genl_info struct to set up
+ * @family: pointer to the genetlink family
+ * @cmd: command to be used in the notification
+ *
+ * Initialize a locally declared struct genl_info to pass to various APIs.
+ * Intended to be used when creating notifications.
+ */
+static inline void
+genl_info_init_ntf(struct genl_info *info, const struct genl_family *family,
+ u8 cmd)
+{
+ struct genlmsghdr *hdr = (void *) &info->user_ptr[0];
+
+ memset(info, 0, sizeof(*info));
+ info->family = family;
+ info->genlhdr = hdr;
+ hdr->cmd = cmd;
+}
+
+static inline bool genl_info_is_ntf(const struct genl_info *info)
+{
+ return !info->nlhdr;
+}
+
int genl_register_family(struct genl_family *family);
int genl_unregister_family(const struct genl_family *family);
void genl_notify(const struct genl_family *family, struct sk_buff *skb,
@@ -278,6 +304,32 @@ void genl_notify(const struct genl_family *family, struct sk_buff *skb,
void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
const struct genl_family *family, int flags, u8 cmd);
+static inline void *
+__genlmsg_iput(struct sk_buff *skb, const struct genl_info *info, int flags)
+{
+ return genlmsg_put(skb, info->snd_portid, info->snd_seq, info->family,
+ flags, info->genlhdr->cmd);
+}
+
+/**
+ * genlmsg_iput - start genetlink message based on genl_info
+ * @skb: skb in which message header will be placed
+ * @info: genl_info as provided to do/dump handlers
+ *
+ * Convenience wrapper which starts a genetlink message based on
+ * information in user request. @info should be either the struct passed
+ * by genetlink core to do/dump handlers (when constructing replies to
+ * such requests) or a struct initialized by genl_info_init_ntf()
+ * when constructing notifications.
+ *
+ * Returns pointer to new genetlink header.
+ */
+static inline void *
+genlmsg_iput(struct sk_buff *skb, const struct genl_info *info)
+{
+ return __genlmsg_iput(skb, info, 0);
+}
+
/**
* genlmsg_nlhdr - Obtain netlink header from user specified header
* @user_hdr: user header as returned from genlmsg_put()
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH net-next v2 07/10] genetlink: add genlmsg_iput() API
2023-08-10 23:38 ` [PATCH net-next v2 07/10] genetlink: add genlmsg_iput() API Jakub Kicinski
@ 2023-08-11 6:44 ` Jiri Pirko
0 siblings, 0 replies; 18+ messages in thread
From: Jiri Pirko @ 2023-08-11 6:44 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, johannes
Fri, Aug 11, 2023 at 01:38:42AM CEST, kuba@kernel.org wrote:
>Add some APIs and helpers required for convenient construction
>of replies and notifications based on struct genl_info.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> include/net/genetlink.h | 54 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 53 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/genetlink.h b/include/net/genetlink.h
>index 6b858c4cba5b..e18a4c0d69ee 100644
>--- a/include/net/genetlink.h
>+++ b/include/net/genetlink.h
>@@ -113,7 +113,7 @@ struct genl_info {
> struct netlink_ext_ack *extack;
> };
>
>-static inline struct net *genl_info_net(struct genl_info *info)
>+static inline struct net *genl_info_net(const struct genl_info *info)
> {
> return read_pnet(&info->_net);
> }
>@@ -270,6 +270,32 @@ genl_info_dump(struct netlink_callback *cb)
> return &genl_dumpit_info(cb)->info;
> }
>
>+/**
>+ * genl_info_init_ntf() - initialize genl_info for notifications
>+ * @info: genl_info struct to set up
>+ * @family: pointer to the genetlink family
>+ * @cmd: command to be used in the notification
>+ *
>+ * Initialize a locally declared struct genl_info to pass to various APIs.
>+ * Intended to be used when creating notifications.
>+ */
>+static inline void
>+genl_info_init_ntf(struct genl_info *info, const struct genl_family *family,
>+ u8 cmd)
>+{
>+ struct genlmsghdr *hdr = (void *) &info->user_ptr[0];
>+
>+ memset(info, 0, sizeof(*info));
>+ info->family = family;
>+ info->genlhdr = hdr;
>+ hdr->cmd = cmd;
>+}
This looks good. Please see my comment to genl_info_is_ntf() in the
reply to the last patch. Anyway, if you decide to keep it, I'm fine:
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>+
>+static inline bool genl_info_is_ntf(const struct genl_info *info)
>+{
>+ return !info->nlhdr;
>+}
>+
> int genl_register_family(struct genl_family *family);
> int genl_unregister_family(const struct genl_family *family);
> void genl_notify(const struct genl_family *family, struct sk_buff *skb,
>@@ -278,6 +304,32 @@ void genl_notify(const struct genl_family *family, struct sk_buff *skb,
> void *genlmsg_put(struct sk_buff *skb, u32 portid, u32 seq,
> const struct genl_family *family, int flags, u8 cmd);
>
>+static inline void *
>+__genlmsg_iput(struct sk_buff *skb, const struct genl_info *info, int flags)
>+{
>+ return genlmsg_put(skb, info->snd_portid, info->snd_seq, info->family,
>+ flags, info->genlhdr->cmd);
>+}
>+
>+/**
>+ * genlmsg_iput - start genetlink message based on genl_info
>+ * @skb: skb in which message header will be placed
>+ * @info: genl_info as provided to do/dump handlers
>+ *
>+ * Convenience wrapper which starts a genetlink message based on
>+ * information in user request. @info should be either the struct passed
>+ * by genetlink core to do/dump handlers (when constructing replies to
>+ * such requests) or a struct initialized by genl_info_init_ntf()
>+ * when constructing notifications.
>+ *
>+ * Returns pointer to new genetlink header.
>+ */
>+static inline void *
>+genlmsg_iput(struct sk_buff *skb, const struct genl_info *info)
>+{
>+ return __genlmsg_iput(skb, info, 0);
>+}
>+
> /**
> * genlmsg_nlhdr - Obtain netlink header from user specified header
> * @user_hdr: user header as returned from genlmsg_put()
>--
>2.41.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next v2 08/10] netdev-genl: use struct genl_info for reply construction
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (6 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 07/10] genetlink: add genlmsg_iput() API Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-11 6:44 ` Jiri Pirko
2023-08-10 23:38 ` [PATCH net-next v2 09/10] ethtool: netlink: simplify arguments to ethnl_default_parse() Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data Jakub Kicinski
9 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski, lorenzo
Use the just added APIs to make the code simpler.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: lorenzo@kernel.org
---
net/core/netdev-genl.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 797c813c7c77..c1aea8b756b6 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -10,11 +10,11 @@
static int
netdev_nl_dev_fill(struct net_device *netdev, struct sk_buff *rsp,
- u32 portid, u32 seq, int flags, u32 cmd)
+ const struct genl_info *info)
{
void *hdr;
- hdr = genlmsg_put(rsp, portid, seq, &netdev_nl_family, flags, cmd);
+ hdr = genlmsg_iput(rsp, info);
if (!hdr)
return -EMSGSIZE;
@@ -41,17 +41,20 @@ netdev_nl_dev_fill(struct net_device *netdev, struct sk_buff *rsp,
static void
netdev_genl_dev_notify(struct net_device *netdev, int cmd)
{
+ struct genl_info info;
struct sk_buff *ntf;
if (!genl_has_listeners(&netdev_nl_family, dev_net(netdev),
NETDEV_NLGRP_MGMT))
return;
+ genl_info_init_ntf(&info, &netdev_nl_family, cmd);
+
ntf = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!ntf)
return;
- if (netdev_nl_dev_fill(netdev, ntf, 0, 0, 0, cmd)) {
+ if (netdev_nl_dev_fill(netdev, ntf, &info)) {
nlmsg_free(ntf);
return;
}
@@ -80,8 +83,7 @@ int netdev_nl_dev_get_doit(struct sk_buff *skb, struct genl_info *info)
netdev = __dev_get_by_index(genl_info_net(info), ifindex);
if (netdev)
- err = netdev_nl_dev_fill(netdev, rsp, info->snd_portid,
- info->snd_seq, 0, info->genlhdr->cmd);
+ err = netdev_nl_dev_fill(netdev, rsp, info);
else
err = -ENODEV;
@@ -105,10 +107,7 @@ int netdev_nl_dev_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
rtnl_lock();
for_each_netdev_dump(net, netdev, cb->args[0]) {
- err = netdev_nl_dev_fill(netdev, skb,
- NETLINK_CB(cb->skb).portid,
- cb->nlh->nlmsg_seq, 0,
- NETDEV_CMD_DEV_GET);
+ err = netdev_nl_dev_fill(netdev, skb, genl_info_dump(cb));
if (err < 0)
break;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 09/10] ethtool: netlink: simplify arguments to ethnl_default_parse()
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (7 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 08/10] netdev-genl: use struct genl_info for reply construction Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-10 23:38 ` [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data Jakub Kicinski
9 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Jiri Pirko, mkubecek
Pass struct genl_info directly instead of its members.
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: mkubecek@suse.cz
---
net/ethtool/netlink.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 9fc7c41f4786..f7b3171a0aad 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -316,10 +316,8 @@ static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
/**
* ethnl_default_parse() - Parse request message
* @req_info: pointer to structure to put data into
- * @tb: parsed attributes
- * @net: request netns
+ * @info: genl_info from the request
* @request_ops: struct request_ops for request type
- * @extack: netlink extack for error reporting
* @require_dev: fail if no device identified in header
*
* Parse universal request header and call request specific ->parse_request()
@@ -328,19 +326,21 @@ static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
* Return: 0 on success or negative error code
*/
static int ethnl_default_parse(struct ethnl_req_info *req_info,
- struct nlattr **tb, struct net *net,
+ const struct genl_info *info,
const struct ethnl_request_ops *request_ops,
- struct netlink_ext_ack *extack, bool require_dev)
+ bool require_dev)
{
+ struct nlattr **tb = info->attrs;
int ret;
ret = ethnl_parse_header_dev_get(req_info, tb[request_ops->hdr_attr],
- net, extack, require_dev);
+ genl_info_net(info), info->extack,
+ require_dev);
if (ret < 0)
return ret;
if (request_ops->parse_request) {
- ret = request_ops->parse_request(req_info, tb, extack);
+ ret = request_ops->parse_request(req_info, tb, info->extack);
if (ret < 0)
return ret;
}
@@ -393,8 +393,7 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
}
- ret = ethnl_default_parse(req_info, info->attrs, genl_info_net(info),
- ops, info->extack, !ops->allow_nodev_do);
+ ret = ethnl_default_parse(req_info, info, ops, !ops->allow_nodev_do);
if (ret < 0)
goto err_dev;
ethnl_init_reply_data(reply_data, ops, req_info->dev);
@@ -538,9 +537,7 @@ static int ethnl_default_start(struct netlink_callback *cb)
goto free_req_info;
}
- ret = ethnl_default_parse(req_info, info->info.attrs,
- sock_net(cb->skb->sk),
- ops, cb->extack, false);
+ ret = ethnl_default_parse(req_info, &info->info, ops, false);
if (req_info->dev) {
/* We ignore device specification in dump requests but as the
* same parser as for non-dump (doit) requests is used, it
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data
2023-08-10 23:38 [PATCH net-next v2 00/10] genetlink: provide struct genl_info to dumps Jakub Kicinski
` (8 preceding siblings ...)
2023-08-10 23:38 ` [PATCH net-next v2 09/10] ethtool: netlink: simplify arguments to ethnl_default_parse() Jakub Kicinski
@ 2023-08-10 23:38 ` Jakub Kicinski
2023-08-11 6:42 ` Jiri Pirko
9 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-10 23:38 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, jiri, johannes, Jakub Kicinski,
Vladimir Oltean, gal, tariqt, lucien.xin, f.fainelli, andrew,
simon.horman, linux, mkubecek
We had a number of bugs in the past because developers forgot
to fully test dumps, which pass NULL as info to .prepare_data.
.prepare_data implementations would try to access info->extack
leading to a null-deref.
Now that dumps and notifications can access struct genl_info
we can pass it in, and remove the info null checks.
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # pause
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: gal@nvidia.com
CC: tariqt@nvidia.com
CC: lucien.xin@gmail.com
CC: f.fainelli@gmail.com
CC: andrew@lunn.ch
CC: simon.horman@corigine.com
CC: linux@rempel-privat.de
CC: mkubecek@suse.cz
CC: johannes@sipsolutions.net
---
net/ethtool/channels.c | 2 +-
net/ethtool/coalesce.c | 6 +++---
net/ethtool/debug.c | 2 +-
net/ethtool/eee.c | 2 +-
net/ethtool/eeprom.c | 9 ++++-----
net/ethtool/features.c | 2 +-
net/ethtool/fec.c | 2 +-
net/ethtool/linkinfo.c | 2 +-
net/ethtool/linkmodes.c | 2 +-
net/ethtool/linkstate.c | 2 +-
net/ethtool/mm.c | 2 +-
net/ethtool/module.c | 5 ++---
net/ethtool/netlink.c | 13 ++++++++-----
net/ethtool/netlink.h | 2 +-
net/ethtool/pause.c | 5 ++---
net/ethtool/phc_vclocks.c | 2 +-
net/ethtool/plca.c | 4 ++--
net/ethtool/privflags.c | 2 +-
net/ethtool/pse-pd.c | 6 +++---
net/ethtool/rings.c | 5 ++---
| 3 ++-
net/ethtool/stats.c | 5 ++---
net/ethtool/strset.c | 2 +-
net/ethtool/tsinfo.c | 2 +-
net/ethtool/wol.c | 5 +++--
25 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index 61c40e889a4d..7b4bbd674bae 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -24,7 +24,7 @@ const struct nla_policy ethnl_channels_get_policy[] = {
static int channels_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct channels_reply_data *data = CHANNELS_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
index 01a59ce211c8..83112c1a71ae 100644
--- a/net/ethtool/coalesce.c
+++ b/net/ethtool/coalesce.c
@@ -59,10 +59,9 @@ const struct nla_policy ethnl_coalesce_get_policy[] = {
static int coalesce_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct coalesce_reply_data *data = COALESCE_REPDATA(reply_base);
- struct netlink_ext_ack *extack = info ? info->extack : NULL;
struct net_device *dev = reply_base->dev;
int ret;
@@ -73,7 +72,8 @@ static int coalesce_prepare_data(const struct ethnl_req_info *req_base,
if (ret < 0)
return ret;
ret = dev->ethtool_ops->get_coalesce(dev, &data->coalesce,
- &data->kernel_coalesce, extack);
+ &data->kernel_coalesce,
+ info->extack);
ethnl_ops_complete(dev);
return ret;
diff --git a/net/ethtool/debug.c b/net/ethtool/debug.c
index e4369769817e..0b2dea56d461 100644
--- a/net/ethtool/debug.c
+++ b/net/ethtool/debug.c
@@ -23,7 +23,7 @@ const struct nla_policy ethnl_debug_get_policy[] = {
static int debug_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct debug_reply_data *data = DEBUG_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index 42104bcb0e47..2853394d06a8 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -26,7 +26,7 @@ const struct nla_policy ethnl_eee_get_policy[] = {
static int eee_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct eee_reply_data *data = EEE_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 49c0a2a77f02..6209c3a9c8f7 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -51,8 +51,7 @@ static int fallback_set_params(struct eeprom_req_info *request,
}
static int eeprom_fallback(struct eeprom_req_info *request,
- struct eeprom_reply_data *reply,
- struct genl_info *info)
+ struct eeprom_reply_data *reply)
{
struct net_device *dev = reply->base.dev;
struct ethtool_modinfo modinfo = {0};
@@ -103,7 +102,7 @@ static int get_module_eeprom_by_page(struct net_device *dev,
static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct eeprom_reply_data *reply = MODULE_EEPROM_REPDATA(reply_base);
struct eeprom_req_info *request = MODULE_EEPROM_REQINFO(req_base);
@@ -124,7 +123,7 @@ static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
if (ret)
goto err_free;
- ret = get_module_eeprom_by_page(dev, &page_data, info ? info->extack : NULL);
+ ret = get_module_eeprom_by_page(dev, &page_data, info->extack);
if (ret < 0)
goto err_ops;
@@ -140,7 +139,7 @@ static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
kfree(page_data.data);
if (ret == -EOPNOTSUPP)
- return eeprom_fallback(request, reply, info);
+ return eeprom_fallback(request, reply);
return ret;
}
diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index 55d449a2d3fc..a79af8c25a07 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -35,7 +35,7 @@ static void ethnl_features_to_bitmap32(u32 *dest, netdev_features_t src)
static int features_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct features_reply_data *data = FEATURES_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/fec.c b/net/ethtool/fec.c
index 0d9a3d153170..e7d3f2c352a3 100644
--- a/net/ethtool/fec.c
+++ b/net/ethtool/fec.c
@@ -92,7 +92,7 @@ fec_stats_recalc(struct fec_stat_grp *grp, struct ethtool_fec_stat *stats)
static int fec_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
__ETHTOOL_DECLARE_LINK_MODE_MASK(active_fec_modes) = {};
struct fec_reply_data *data = FEC_REPDATA(reply_base);
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index 310dfe63292a..5c317d23787b 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -23,7 +23,7 @@ const struct nla_policy ethnl_linkinfo_get_policy[] = {
static int linkinfo_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct linkinfo_reply_data *data = LINKINFO_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 20165e07ef90..b2591db49f7d 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -27,7 +27,7 @@ const struct nla_policy ethnl_linkmodes_get_policy[] = {
static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 2158c17a0b32..b2de2108b356 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -81,7 +81,7 @@ static int linkstate_get_link_ext_state(struct net_device *dev,
static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct linkstate_reply_data *data = LINKSTATE_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
index 4058a557b5a4..2816bb23c3ad 100644
--- a/net/ethtool/mm.c
+++ b/net/ethtool/mm.c
@@ -27,7 +27,7 @@ const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1] = {
static int mm_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct mm_reply_data *data = MM_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index e0d539b21423..ceb575efc290 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -38,10 +38,9 @@ static int module_get_power_mode(struct net_device *dev,
static int module_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct module_reply_data *data = MODULE_REPDATA(reply_base);
- struct netlink_ext_ack *extack = info ? info->extack : NULL;
struct net_device *dev = reply_base->dev;
int ret;
@@ -49,7 +48,7 @@ static int module_prepare_data(const struct ethnl_req_info *req_base,
if (ret < 0)
return ret;
- ret = module_get_power_mode(dev, data, extack);
+ ret = module_get_power_mode(dev, data, info->extack);
if (ret < 0)
goto out_complete;
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index f7b3171a0aad..3bbd5afb7b31 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -444,12 +444,12 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
const struct ethnl_dump_ctx *ctx,
- struct netlink_callback *cb)
+ const struct genl_info *info)
{
void *ehdr;
int ret;
- ehdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
+ ehdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
ðtool_genl_family, NLM_F_MULTI,
ctx->ops->reply_cmd);
if (!ehdr)
@@ -457,7 +457,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
ethnl_init_reply_data(ctx->reply_data, ctx->ops, dev);
rtnl_lock();
- ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, NULL);
+ ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, info);
rtnl_unlock();
if (ret < 0)
goto out;
@@ -495,7 +495,7 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
dev_hold(dev);
rtnl_unlock();
- ret = ethnl_default_dump_one(skb, dev, ctx, cb);
+ ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
rtnl_lock();
dev_put(dev);
@@ -647,11 +647,14 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
struct ethnl_reply_data *reply_data;
const struct ethnl_request_ops *ops;
struct ethnl_req_info *req_info;
+ struct genl_info info;
struct sk_buff *skb;
void *reply_payload;
int reply_len;
int ret;
+ genl_info_init_ntf(&info, ðtool_genl_family, cmd);
+
if (WARN_ONCE(cmd > ETHTOOL_MSG_KERNEL_MAX ||
!ethnl_default_notify_ops[cmd],
"unexpected notification type %u\n", cmd))
@@ -670,7 +673,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS;
ethnl_init_reply_data(reply_data, ops, dev);
- ret = ops->prepare_data(req_info, reply_data, NULL);
+ ret = ops->prepare_data(req_info, reply_data, &info);
if (ret < 0)
goto err_cleanup;
ret = ops->reply_size(req_info, reply_data);
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 79424b34b553..9a333a8d04c1 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -355,7 +355,7 @@ struct ethnl_request_ops {
struct netlink_ext_ack *extack);
int (*prepare_data)(const struct ethnl_req_info *req_info,
struct ethnl_reply_data *reply_data,
- struct genl_info *info);
+ const struct genl_info *info);
int (*reply_size)(const struct ethnl_req_info *req_info,
const struct ethnl_reply_data *reply_data);
int (*fill_reply)(struct sk_buff *skb,
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index 6657d0b888d8..f7c847aeb1a2 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -51,10 +51,9 @@ static int pause_parse_request(struct ethnl_req_info *req_base,
static int pause_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
const struct pause_req_info *req_info = PAUSE_REQINFO(req_base);
- struct netlink_ext_ack *extack = info ? info->extack : NULL;
struct pause_reply_data *data = PAUSE_REPDATA(reply_base);
enum ethtool_mac_stats_src src = req_info->src;
struct net_device *dev = reply_base->dev;
@@ -74,7 +73,7 @@ static int pause_prepare_data(const struct ethnl_req_info *req_base,
if ((src == ETHTOOL_MAC_STATS_SRC_EMAC ||
src == ETHTOOL_MAC_STATS_SRC_PMAC) &&
!__ethtool_dev_mm_supported(dev)) {
- NL_SET_ERR_MSG_MOD(extack,
+ NL_SET_ERR_MSG_MOD(info->extack,
"Device does not support MAC merge layer");
ethnl_ops_complete(dev);
return -EOPNOTSUPP;
diff --git a/net/ethtool/phc_vclocks.c b/net/ethtool/phc_vclocks.c
index 637b2f5297d5..cadaabed60bd 100644
--- a/net/ethtool/phc_vclocks.c
+++ b/net/ethtool/phc_vclocks.c
@@ -24,7 +24,7 @@ const struct nla_policy ethnl_phc_vclocks_get_policy[] = {
static int phc_vclocks_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct phc_vclocks_reply_data *data = PHC_VCLOCKS_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index 5a8cab4df0c9..b238a1afe9ae 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -40,7 +40,7 @@ const struct nla_policy ethnl_plca_get_cfg_policy[] = {
static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct plca_reply_data *data = PLCA_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
@@ -183,7 +183,7 @@ const struct nla_policy ethnl_plca_get_status_policy[] = {
static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct plca_reply_data *data = PLCA_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/privflags.c b/net/ethtool/privflags.c
index 23264a1ebf12..297be6a13ab9 100644
--- a/net/ethtool/privflags.c
+++ b/net/ethtool/privflags.c
@@ -57,7 +57,7 @@ static int ethnl_get_priv_flags_info(struct net_device *dev,
static int privflags_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct privflags_reply_data *data = PRIVFLAGS_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 530b8b99e6df..cc478af77111 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -53,8 +53,8 @@ static int pse_get_pse_attributes(struct net_device *dev,
}
static int pse_prepare_data(const struct ethnl_req_info *req_base,
- struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ struct ethnl_reply_data *reply_base,
+ const struct genl_info *info)
{
struct pse_reply_data *data = PSE_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
@@ -64,7 +64,7 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
if (ret < 0)
return ret;
- ret = pse_get_pse_attributes(dev, info ? info->extack : NULL, data);
+ ret = pse_get_pse_attributes(dev, info->extack, data);
ethnl_ops_complete(dev);
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 1c4972526142..fb09f774ea01 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -24,10 +24,9 @@ const struct nla_policy ethnl_rings_get_policy[] = {
static int rings_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct rings_reply_data *data = RINGS_REPDATA(reply_base);
- struct netlink_ext_ack *extack = info ? info->extack : NULL;
struct net_device *dev = reply_base->dev;
int ret;
@@ -39,7 +38,7 @@ static int rings_prepare_data(const struct ethnl_req_info *req_base,
if (ret < 0)
return ret;
dev->ethtool_ops->get_ringparam(dev, &data->ringparam,
- &data->kernel_ringparam, extack);
+ &data->kernel_ringparam, info->extack);
ethnl_ops_complete(dev);
return 0;
--git a/net/ethtool/rss.c b/net/ethtool/rss.c
index be260ab34e58..5764202e6cb6 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -42,7 +42,8 @@ rss_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
static int
rss_prepare_data(const struct ethnl_req_info *req_base,
- struct ethnl_reply_data *reply_base, struct genl_info *info)
+ struct ethnl_reply_data *reply_base,
+ const struct genl_info *info)
{
struct rss_reply_data *data = RSS_REPDATA(reply_base);
struct rss_req_info *request = RSS_REQINFO(req_base);
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 010ed19ccc99..912f0c4fff2f 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -114,10 +114,9 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
static int stats_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
const struct stats_req_info *req_info = STATS_REQINFO(req_base);
- struct netlink_ext_ack *extack = info ? info->extack : NULL;
struct stats_reply_data *data = STATS_REPDATA(reply_base);
enum ethtool_mac_stats_src src = req_info->src;
struct net_device *dev = reply_base->dev;
@@ -130,7 +129,7 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
if ((src == ETHTOOL_MAC_STATS_SRC_EMAC ||
src == ETHTOOL_MAC_STATS_SRC_PMAC) &&
!__ethtool_dev_mm_supported(dev)) {
- NL_SET_ERR_MSG_MOD(extack,
+ NL_SET_ERR_MSG_MOD(info->extack,
"Device does not support MAC merge layer");
ethnl_ops_complete(dev);
return -EOPNOTSUPP;
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 3f7de54d85fb..c678b484a079 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -274,7 +274,7 @@ static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
static int strset_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
const struct strset_req_info *req_info = STRSET_REQINFO(req_base);
struct strset_reply_data *data = STRSET_REPDATA(reply_base);
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 63b5814bd460..9daed0aab162 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -25,7 +25,7 @@ const struct nla_policy ethnl_tsinfo_get_policy[] = {
static int tsinfo_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct tsinfo_reply_data *data = TSINFO_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index a4a43d9e6e9d..5f2bc8e549af 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {
static int wol_prepare_data(const struct ethnl_req_info *req_base,
struct ethnl_reply_data *reply_base,
- struct genl_info *info)
+ const struct genl_info *info)
{
struct wol_reply_data *data = WOL_REPDATA(reply_base);
struct net_device *dev = reply_base->dev;
@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
dev->ethtool_ops->get_wol(dev, &data->wol);
ethnl_ops_complete(dev);
/* do not include password in notifications */
- data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
+ data->show_sopass = genl_info_is_ntf(info) &&
+ (data->wol.supported & WAKE_MAGICSECURE);
return 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data
2023-08-10 23:38 ` [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data Jakub Kicinski
@ 2023-08-11 6:42 ` Jiri Pirko
2023-08-11 7:13 ` Michal Kubecek
0 siblings, 1 reply; 18+ messages in thread
From: Jiri Pirko @ 2023-08-11 6:42 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, johannes, Vladimir Oltean, gal,
tariqt, lucien.xin, f.fainelli, andrew, simon.horman, linux,
mkubecek
Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@kernel.org wrote:
[...]
>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>index f7b3171a0aad..3bbd5afb7b31 100644
>--- a/net/ethtool/netlink.c
>+++ b/net/ethtool/netlink.c
>@@ -444,12 +444,12 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
>
> static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
> const struct ethnl_dump_ctx *ctx,
>- struct netlink_callback *cb)
>+ const struct genl_info *info)
> {
> void *ehdr;
> int ret;
>
>- ehdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
>+ ehdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
> ðtool_genl_family, NLM_F_MULTI,
> ctx->ops->reply_cmd);
> if (!ehdr)
>@@ -457,7 +457,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
>
> ethnl_init_reply_data(ctx->reply_data, ctx->ops, dev);
> rtnl_lock();
>- ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, NULL);
>+ ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, info);
> rtnl_unlock();
> if (ret < 0)
> goto out;
>@@ -495,7 +495,7 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
> dev_hold(dev);
> rtnl_unlock();
>
>- ret = ethnl_default_dump_one(skb, dev, ctx, cb);
>+ ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
>
> rtnl_lock();
> dev_put(dev);
>@@ -647,11 +647,14 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
> struct ethnl_reply_data *reply_data;
> const struct ethnl_request_ops *ops;
> struct ethnl_req_info *req_info;
>+ struct genl_info info;
> struct sk_buff *skb;
> void *reply_payload;
> int reply_len;
> int ret;
>
>+ genl_info_init_ntf(&info, ðtool_genl_family, cmd);
>+
> if (WARN_ONCE(cmd > ETHTOOL_MSG_KERNEL_MAX ||
> !ethnl_default_notify_ops[cmd],
> "unexpected notification type %u\n", cmd))
>@@ -670,7 +673,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
> req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS;
>
> ethnl_init_reply_data(reply_data, ops, dev);
>- ret = ops->prepare_data(req_info, reply_data, NULL);
>+ ret = ops->prepare_data(req_info, reply_data, &info);
> if (ret < 0)
> goto err_cleanup;
> ret = ops->reply_size(req_info, reply_data);
[...]
>@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {
>
> static int wol_prepare_data(const struct ethnl_req_info *req_base,
> struct ethnl_reply_data *reply_base,
>- struct genl_info *info)
>+ const struct genl_info *info)
> {
> struct wol_reply_data *data = WOL_REPDATA(reply_base);
> struct net_device *dev = reply_base->dev;
>@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
> dev->ethtool_ops->get_wol(dev, &data->wol);
> ethnl_ops_complete(dev);
> /* do not include password in notifications */
>- data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
>+ data->show_sopass = genl_info_is_ntf(info) &&
>+ (data->wol.supported & WAKE_MAGICSECURE);
I believe that you are missing "!" here:
data->show_sopass = !genl_info_is_ntf(info) &&
(data->wol.supported & WAKE_MAGICSECURE);
But, you are changing the output for dumpit if I'm not mistaken.
ethnl_default_dump_one() currently calls this with info==NULL too, not
only ethnl_default_notify().
Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
user is here and I doubt there ever going to be any other. This
conditional per-op attr fill seems a bit odd.
Can't you handle this in side ethtool somehow? IDK :/
>
> return 0;
> }
>--
>2.41.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data
2023-08-11 6:42 ` Jiri Pirko
@ 2023-08-11 7:13 ` Michal Kubecek
2023-08-11 7:29 ` Jiri Pirko
0 siblings, 1 reply; 18+ messages in thread
From: Michal Kubecek @ 2023-08-11 7:13 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, davem, netdev, edumazet, pabeni, johannes,
Vladimir Oltean, gal, tariqt, lucien.xin, f.fainelli, andrew,
simon.horman, linux
[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]
On Fri, Aug 11, 2023 at 08:42:45AM +0200, Jiri Pirko wrote:
> Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@kernel.org wrote:
> >@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {
> >
> > static int wol_prepare_data(const struct ethnl_req_info *req_base,
> > struct ethnl_reply_data *reply_base,
> >- struct genl_info *info)
> >+ const struct genl_info *info)
> > {
> > struct wol_reply_data *data = WOL_REPDATA(reply_base);
> > struct net_device *dev = reply_base->dev;
> >@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
> > dev->ethtool_ops->get_wol(dev, &data->wol);
> > ethnl_ops_complete(dev);
> > /* do not include password in notifications */
> >- data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
> >+ data->show_sopass = genl_info_is_ntf(info) &&
> >+ (data->wol.supported & WAKE_MAGICSECURE);
>
> I believe that you are missing "!" here:
> data->show_sopass = !genl_info_is_ntf(info) &&
> (data->wol.supported & WAKE_MAGICSECURE);
Agreed.
> But, you are changing the output for dumpit if I'm not mistaken.
> ethnl_default_dump_one() currently calls this with info==NULL too, not
> only ethnl_default_notify().
I would rather see this as a fix. Not showing the password in dumps made
little sense as it meant the dump output was different from single
device queries. It was the price to pay for inability to distinguish
between a dump and a notification.
IIRC the early versions submitted went even further and did not set
GENL_UNS_ADMIN_PERM for ETHTOOL_MSG_WOL_GET and only omitted the
password when the request came from an unprivileged process (so that
unprivileged processes could still query the rest of WoL information)
but this was dropped during the review as an unnecessary complication.
> Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
> user is here and I doubt there ever going to be any other. This
> conditional per-op attr fill seems a bit odd.
>
> Can't you handle this in side ethtool somehow? IDK :/
I don't think so. The point here is that notification can be seen by any
unprivileged process so as long as we agree that those should not see
the wake up passwords, we must not include the password in them. While
ethtool could certanly drop the password from its output, any other
utility parsing the notifications (or even patched ethtool) could still
show it to anyone.
Michal
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data
2023-08-11 7:13 ` Michal Kubecek
@ 2023-08-11 7:29 ` Jiri Pirko
2023-08-11 22:24 ` Jakub Kicinski
0 siblings, 1 reply; 18+ messages in thread
From: Jiri Pirko @ 2023-08-11 7:29 UTC (permalink / raw)
To: Michal Kubecek
Cc: Jakub Kicinski, davem, netdev, edumazet, pabeni, johannes,
Vladimir Oltean, gal, tariqt, lucien.xin, f.fainelli, andrew,
simon.horman, linux
Fri, Aug 11, 2023 at 09:13:24AM CEST, mkubecek@suse.cz wrote:
>On Fri, Aug 11, 2023 at 08:42:45AM +0200, Jiri Pirko wrote:
>> Fri, Aug 11, 2023 at 01:38:45AM CEST, kuba@kernel.org wrote:
>> >@@ -24,7 +24,7 @@ const struct nla_policy ethnl_wol_get_policy[] = {
>> >
>> > static int wol_prepare_data(const struct ethnl_req_info *req_base,
>> > struct ethnl_reply_data *reply_base,
>> >- struct genl_info *info)
>> >+ const struct genl_info *info)
>> > {
>> > struct wol_reply_data *data = WOL_REPDATA(reply_base);
>> > struct net_device *dev = reply_base->dev;
>> >@@ -39,7 +39,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
>> > dev->ethtool_ops->get_wol(dev, &data->wol);
>> > ethnl_ops_complete(dev);
>> > /* do not include password in notifications */
>> >- data->show_sopass = info && (data->wol.supported & WAKE_MAGICSECURE);
>> >+ data->show_sopass = genl_info_is_ntf(info) &&
>> >+ (data->wol.supported & WAKE_MAGICSECURE);
>>
>> I believe that you are missing "!" here:
>> data->show_sopass = !genl_info_is_ntf(info) &&
>> (data->wol.supported & WAKE_MAGICSECURE);
>
>Agreed.
>
>> But, you are changing the output for dumpit if I'm not mistaken.
>> ethnl_default_dump_one() currently calls this with info==NULL too, not
>> only ethnl_default_notify().
>
>I would rather see this as a fix. Not showing the password in dumps made
>little sense as it meant the dump output was different from single
>device queries. It was the price to pay for inability to distinguish
>between a dump and a notification.
>
>IIRC the early versions submitted went even further and did not set
>GENL_UNS_ADMIN_PERM for ETHTOOL_MSG_WOL_GET and only omitted the
>password when the request came from an unprivileged process (so that
>unprivileged processes could still query the rest of WoL information)
>but this was dropped during the review as an unnecessary complication.
>
>> Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
>> user is here and I doubt there ever going to be any other. This
>> conditional per-op attr fill seems a bit odd.
>>
>> Can't you handle this in side ethtool somehow? IDK :/
>
>I don't think so. The point here is that notification can be seen by any
>unprivileged process so as long as we agree that those should not see
>the wake up passwords, we must not include the password in them. While
>ethtool could certanly drop the password from its output, any other
>utility parsing the notifications (or even patched ethtool) could still
>show it to anyone.
Yeah, the question is, if it is a good design to have one CMD type
to conditionally send sensitive data. I would argue that sensitive data
could be sent over separate CMD with no notifier for it.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v2 10/10] ethtool: netlink: always pass genl_info to .prepare_data
2023-08-11 7:29 ` Jiri Pirko
@ 2023-08-11 22:24 ` Jakub Kicinski
0 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-08-11 22:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: Michal Kubecek, davem, netdev, edumazet, pabeni, johannes,
Vladimir Oltean, gal, tariqt, lucien.xin, f.fainelli, andrew,
simon.horman, linux
On Fri, 11 Aug 2023 09:29:58 +0200 Jiri Pirko wrote:
> >> Anyway, the genl_info_is_ntf() itself seems a bit odd to me. The only
> >> user is here and I doubt there ever going to be any other. This
> >> conditional per-op attr fill seems a bit odd.
> >>
> >> Can't you handle this in side ethtool somehow? IDK :/
> >
> >I don't think so. The point here is that notification can be seen by any
> >unprivileged process so as long as we agree that those should not see
> >the wake up passwords, we must not include the password in them. While
> >ethtool could certanly drop the password from its output, any other
> >utility parsing the notifications (or even patched ethtool) could still
> >show it to anyone.
>
> Yeah, the question is, if it is a good design to have one CMD type
> to conditionally send sensitive data. I would argue that sensitive data
> could be sent over separate CMD with no notifier for it.
Good catch!
Hopefully we can address that separately (I mean someone who cares can
send a patch? :)). We had multiple people get surprised by info being
NULL I think the value of the other changes outweighs resolving this
little oddity. I'm going to send a v3 with the bug fixed later.
On the existence of genl_info_is_ntf(), I would rather keep it.
I'm a bit worried someone else will need to know at some point and
will do it based on contents of info directly, which will make
future refactoring risky.
^ permalink raw reply [flat|nested] 18+ messages in thread