From: Sven Eckelmann <sven@narfation.org>
To: b.a.t.m.a.n@lists.open-mesh.org
Cc: "Linus Lüssing" <linus.luessing@c0d3.blue>
Subject: Re: [PATCH v7 1/3] batman-adv: netlink: reduce duplicate code by returning interfaces
Date: Sat, 04 Jan 2025 22:29:38 +0100 [thread overview]
Message-ID: <2132627.atdPhlSkOF@sven-l14> (raw)
In-Reply-To: <20241130005942.24497-2-linus.luessing@c0d3.blue>
[-- Attachment #1: Type: text/plain, Size: 12901 bytes --]
On Saturday, 30 November 2024 00:46:32 GMT+1 Linus Lüssing wrote:
[...]
> @@ -2233,25 +2233,16 @@ int batadv_bla_claim_dump(struct sk_buff *msg, struct netlink_callback *cb)
> {
> struct batadv_hard_iface *primary_if = NULL;
> int portid = NETLINK_CB(cb->skb).portid;
> - struct net *net = sock_net(cb->skb->sk);
> struct net_device *soft_iface;
> struct batadv_hashtable *hash;
> struct batadv_priv *bat_priv;
> int bucket = cb->args[0];
> int idx = cb->args[1];
> - int ifindex;
> int ret = 0;
>
> - ifindex = batadv_netlink_get_ifindex(cb->nlh,
> - BATADV_ATTR_MESH_IFINDEX);
> - if (!ifindex)
> - return -EINVAL;
> -
> - soft_iface = dev_get_by_index(net, ifindex);
> - if (!soft_iface || !batadv_softif_is_valid(soft_iface)) {
> - ret = -ENODEV;
> - goto out;
> - }
> + soft_iface = batadv_netlink_get_softif(cb);
> + if (IS_ERR(soft_iface))
> + return PTR_ERR(soft_iface);
To use these helpers, you need to "#include <linux/err.h>" in various files.
There are also other changes needed. Here an overview:
diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index c38a1ac9..fa8a8246 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -12,6 +12,7 @@
#include <linux/compiler.h>
#include <linux/container_of.h>
#include <linux/crc16.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/gfp.h>
@@ -38,7 +39,6 @@
#include <net/arp.h>
#include <net/genetlink.h>
#include <net/netlink.h>
-#include <net/sock.h>
#include <uapi/linux/batadv_packet.h>
#include <uapi/linux/batman_adv.h>
@@ -47,7 +47,6 @@
#include "log.h"
#include "netlink.h"
#include "originator.h"
-#include "soft-interface.h"
#include "translation-table.h"
static const u8 batadv_announce_mac[4] = {0x43, 0x05, 0x43, 0x05};
diff --git a/net/batman-adv/distributed-arp-table.c b/net/batman-adv/distributed-arp-table.c
index 67bb3b17..ff660758 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -11,6 +11,7 @@
#include <linux/bitops.h>
#include <linux/byteorder/generic.h>
#include <linux/container_of.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/gfp.h>
@@ -36,7 +37,6 @@
#include <net/arp.h>
#include <net/genetlink.h>
#include <net/netlink.h>
-#include <net/sock.h>
#include <uapi/linux/batman_adv.h>
#include "bridge_loop_avoidance.h"
@@ -46,7 +46,6 @@
#include "netlink.h"
#include "originator.h"
#include "send.h"
-#include "soft-interface.h"
#include "translation-table.h"
#include "tvlv.h"
diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index dc57b54b..8afdc711 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@ -10,6 +10,7 @@
#include <linux/atomic.h>
#include <linux/byteorder/generic.h>
#include <linux/container_of.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/gfp.h>
@@ -31,7 +32,6 @@
#include <linux/sprintf.h>
#include <linux/stddef.h>
#include <linux/udp.h>
-#include <net/sock.h>
#include <uapi/linux/batadv_packet.h>
#include <uapi/linux/batman_adv.h>
@@ -40,7 +40,6 @@
#include "netlink.h"
#include "originator.h"
#include "routing.h"
-#include "soft-interface.h"
#include "translation-table.h"
/* These are the offsets of the "hw type" and "hw address length" in the dhcp
diff --git a/net/batman-adv/multicast.c b/net/batman-adv/multicast.c
index 17b302db..5b13010d 100644
--- a/net/batman-adv/multicast.c
+++ b/net/batman-adv/multicast.c
@@ -12,6 +12,7 @@
#include <linux/bug.h>
#include <linux/byteorder/generic.h>
#include <linux/container_of.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/gfp.h>
@@ -46,7 +47,6 @@
#include <net/ip.h>
#include <net/ipv6.h>
#include <net/netlink.h>
-#include <net/sock.h>
#include <uapi/linux/batadv_packet.h>
#include <uapi/linux/batman_adv.h>
@@ -56,7 +56,6 @@
#include "log.h"
#include "netlink.h"
#include "send.h"
-#include "soft-interface.h"
#include "translation-table.h"
#include "tvlv.h"
diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index 145eb110..972003d3 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
@@ -9,6 +9,7 @@
#include <linux/atomic.h>
#include <linux/container_of.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/gfp.h>
@@ -26,9 +27,7 @@
#include <linux/spinlock.h>
#include <linux/stddef.h>
#include <linux/workqueue.h>
-#include <net/sock.h>
#include <uapi/linux/batadv_packet.h>
-#include <uapi/linux/batman_adv.h>
#include "bat_algo.h"
#include "distributed-arp-table.h"
@@ -41,7 +40,6 @@
#include "netlink.h"
#include "network-coding.h"
#include "routing.h"
-#include "soft-interface.h"
#include "translation-table.h"
/* hash class keys */
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index d464569e..a7680858 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -15,6 +15,7 @@
#include <linux/compiler.h>
#include <linux/container_of.h>
#include <linux/crc32c.h>
+#include <linux/err.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/gfp.h>
@@ -39,7 +40,6 @@
#include <linux/workqueue.h>
#include <net/genetlink.h>
#include <net/netlink.h>
-#include <net/sock.h>
#include <uapi/linux/batadv_packet.h>
#include <uapi/linux/batman_adv.h>
[...]
> @@ -1150,23 +1137,17 @@ static int batadv_netlink_set_vlan(struct sk_buff *skb, struct genl_info *info)
> }
>
> /**
> - * batadv_get_softif_from_info() - Retrieve soft interface from genl attributes
> + * batadv_netlink_get_softif_from_info() - Retrieve soft interface from ifindex
> * @net: the applicable net namespace
> - * @info: receiver information
> + * @ifindex: index of the soft interface
> *
> * Return: Pointer to soft interface (with increased refcnt) on success, error
> * pointer on error
> */
> static struct net_device *
> -batadv_get_softif_from_info(struct net *net, struct genl_info *info)
> +batadv_netlink_get_softif_from_ifindex(struct net *net, int ifindex)
I think you wanted to document batadv_netlink_get_softif_from_ifindex here and
not batadv_netlink_get_softif_from_info
[...]
> @@ -1184,28 +1165,61 @@ batadv_get_softif_from_info(struct net *net, struct genl_info *info)
> }
>
> /**
> - * batadv_get_hardif_from_info() - Retrieve hardif from genl attributes
> - * @bat_priv: the bat priv with all the soft interface information
> + * batadv_get_softif_from_info() - Retrieve soft interface from genl attributes
> * @net: the applicable net namespace
> * @info: receiver information
> *
> + * Return: Pointer to soft interface (with increased refcnt) on success, error
> + * pointer on error
> + */
> +static struct net_device *
> +batadv_netlink_get_softif_from_info(struct net *net, struct genl_info *info)
I think you wanted to document batadv_netlink_get_softif_from_info here and
not batadv_get_softif_from_info
[...]
> +/**
> + * batadv_netlink_get_hardif_from_info() - Retrieve hard interface from ifindex
> + * @bat_priv: the bat priv with all the soft interface information
> + * @net: the applicable net namespace
> + * @ifindex: index of the hard interface
> + *
> * Return: Pointer to hard interface (with increased refcnt) on success, error
> * pointer on error
> */
> static struct batadv_hard_iface *
> -batadv_get_hardif_from_info(struct batadv_priv *bat_priv, struct net *net,
> - struct genl_info *info)
> +batadv_netlink_get_hardif_from_ifindex(struct batadv_priv *bat_priv,
> + struct net *net, int ifindex)
> {
I think you wanted to document batadv_netlink_get_hardif_from_ifindex here and
not batadv_netlink_get_hardif_from_info
[...]
> @@ -1229,6 +1243,51 @@ batadv_get_hardif_from_info(struct batadv_priv *bat_priv, struct net *net,
> return ERR_PTR(-EINVAL);
> }
>
> +/**
> + * batadv_get_hardif_from_info() - Retrieve hardif from genl attributes
> + * @bat_priv: the bat priv with all the soft interface information
> + * @net: the applicable net namespace
> + * @info: receiver information
> + *
> + * Return: Pointer to hard interface (with increased refcnt) on success, error
> + * pointer on error
> + */
> +static struct batadv_hard_iface *
> +batadv_netlink_get_hardif_from_info(struct batadv_priv *bat_priv,
> + struct net *net, struct genl_info *info)
I think you wanted to document batadv_netlink_get_hardif_from_info here and
not batadv_get_hardif_from_info
[...]
> diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
> index 8f6dd2c6ee41..9ee9655d9bea 100644
> --- a/net/batman-adv/originator.c
> +++ b/net/batman-adv/originator.c
> @@ -755,24 +755,15 @@ batadv_neigh_node_get_or_create(struct batadv_orig_node *orig_node,
> */
> int batadv_hardif_neigh_dump(struct sk_buff *msg, struct netlink_callback *cb)
> {
> - struct net *net = sock_net(cb->skb->sk);
> struct net_device *soft_iface;
> - struct net_device *hard_iface = NULL;
> - struct batadv_hard_iface *hardif = BATADV_IF_DEFAULT;
> + struct batadv_hard_iface *hard_iface;
hard_iface is no longer initialized but you will use it after a goto (see
below).
> struct batadv_priv *bat_priv;
> struct batadv_hard_iface *primary_if = NULL;
> int ret;
> - int ifindex, hard_ifindex;
>
> - ifindex = batadv_netlink_get_ifindex(cb->nlh, BATADV_ATTR_MESH_IFINDEX);
> - if (!ifindex)
> - return -EINVAL;
> -
> - soft_iface = dev_get_by_index(net, ifindex);
> - if (!soft_iface || !batadv_softif_is_valid(soft_iface)) {
> - ret = -ENODEV;
> - goto out;
> - }
> + soft_iface = batadv_netlink_get_softif(cb);
> + if (IS_ERR(soft_iface))
> + return PTR_ERR(soft_iface);
>
> bat_priv = netdev_priv(soft_iface);
>
> @@ -782,22 +773,14 @@ int batadv_hardif_neigh_dump(struct sk_buff *msg, struct netlink_callback *cb)
> goto out;
> }
The goto here seems to be rather problematic... see below for more details
>
> - hard_ifindex = batadv_netlink_get_ifindex(cb->nlh,
> - BATADV_ATTR_HARD_IFINDEX);
> - if (hard_ifindex) {
> - hard_iface = dev_get_by_index(net, hard_ifindex);
> - if (hard_iface)
> - hardif = batadv_hardif_get_by_netdev(hard_iface);
> -
> - if (!hardif) {
> - ret = -ENODEV;
> + hard_iface = batadv_netlink_get_hardif(bat_priv, cb);
> + if (IS_ERR(hard_iface)) {
> + if (PTR_ERR(hard_iface) != -ENONET) {
> + ret = PTR_ERR(hard_iface);
> goto out;
> }
>
> - if (hardif->soft_iface != soft_iface) {
> - ret = -ENOENT;
> - goto out;
> - }
> + hard_iface = BATADV_IF_DEFAULT;
> }
>
> if (!bat_priv->algo_ops->neigh.dump) {
> @@ -805,13 +788,12 @@ int batadv_hardif_neigh_dump(struct sk_buff *msg, struct netlink_callback *cb)
> goto out;
> }
>
> - bat_priv->algo_ops->neigh.dump(msg, cb, bat_priv, hardif);
> + bat_priv->algo_ops->neigh.dump(msg, cb, bat_priv, hard_iface);
>
> ret = msg->len;
>
> out:
> - batadv_hardif_put(hardif);
> - dev_put(hard_iface);
> + batadv_hardif_put(hard_iface);
> batadv_hardif_put(primary_if);
> dev_put(soft_iface);
You will now give batadv_hardif_put an unitialized value for hard_iface when
following goto is used:
[...]
primary_if = batadv_primary_if_get_selected(bat_priv);
if (!primary_if || primary_if->if_status != BATADV_IF_ACTIVE) {
ret = -ENOENT;
goto out;
}
hard_iface = batadv_netlink_get_hardif(bat_priv, cb);
[...]
This is also the reason why a non-descriptive, single return goto labels
should be avoided [1].
Kind regards,
Sven
[1] https://www.kernel.org/doc/html/v6.12/process/coding-style.html#centralized-exiting-of-functions
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2025-01-04 21:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-29 23:46 [PATCH v7 0/3] batman-adv: increase DAT DHT timeout Linus Lüssing
2024-11-29 23:46 ` [PATCH v7 1/3] batman-adv: netlink: reduce duplicate code by returning interfaces Linus Lüssing
2025-01-04 21:29 ` Sven Eckelmann [this message]
2024-11-29 23:46 ` [PATCH v7 2/3] batman-adv: split DAT cache into DAT cache and DAT DHT Linus Lüssing
2024-11-29 23:46 ` [PATCH v7 3/3] batman-adv: increase DAT DHT timeout Linus Lüssing
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2132627.atdPhlSkOF@sven-l14 \
--to=sven@narfation.org \
--cc=b.a.t.m.a.n@lists.open-mesh.org \
--cc=linus.luessing@c0d3.blue \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.