Re: [PATCH net-next v2 3/3] genetlink: fit NLMSG_DONE into same read() as families
From: Stefano Brivio <hidden>
Date: 2024-03-15 11:48:54
Hi, On Sat, 2 Mar 2024 21:24:08 -0800 Jakub Kicinski [off-list ref] wrote:
Make sure ctrl_fill_info() returns sensible error codes and propagate them out to netlink core. Let netlink core decide when to return skb->len and when to treat the exit as an error. Netlink core does better job at it, if we always return skb->len the core doesn't know when we're done dumping and NLMSG_DONE ends up in a separate read().
While this change is obviously correct, it breaks... well, broken applications that _wrongly_ rely on the fact that NLMSG_DONE is delivered in a separate datagram. This was the (embarrassing) case for passt(1), which I just fixed: https://archives.passt.top/passt-dev/20240315112432.382212-1-sbrivio@redhat.com/ but the "separate" NLMSG_DONE is such an established behaviour, I think, that this might raise a more general concern. From my perspective, I'm just happy that this change revealed the issue, but I wanted to report this anyway in case somebody has similar possible breakages in mind.
quoted hunk ↗ jump to hunk
Reviewed-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: jiri@resnulli.us --- net/netlink/genetlink.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 50ec599a5cff..3b7666944b11 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c@@ -1232,7 +1232,7 @@ static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq, hdr = genlmsg_put(skb, portid, seq, &genl_ctrl, flags, cmd); if (hdr == NULL) - return -1; + return -EMSGSIZE; if (nla_put_string(skb, CTRL_ATTR_FAMILY_NAME, family->name) || nla_put_u16(skb, CTRL_ATTR_FAMILY_ID, family->id) ||@@ -1355,6 +1355,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb) struct net *net = sock_net(skb->sk); int fams_to_skip = cb->args[0]; unsigned int id; + int err = 0; idr_for_each_entry(&genl_fam_idr, rt, id) { if (!rt->netnsok && !net_eq(net, &init_net))@@ -1363,16 +1364,17 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb) if (n++ < fams_to_skip) continue; - if (ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid, - cb->nlh->nlmsg_seq, NLM_F_MULTI, - skb, CTRL_CMD_NEWFAMILY) < 0) { + err = ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid, + cb->nlh->nlmsg_seq, NLM_F_MULTI, + skb, CTRL_CMD_NEWFAMILY); + if (err) { n--; break; } } cb->args[0] = n; - return skb->len; + return err; } static struct sk_buff *ctrl_build_family_msg(const struct genl_family *family,
-- Stefano