diff mbox series

[v1,net,1/3] gtp: Use for_each_netdev_rcu() in gtp_genl_dump_pdp().

Message ID 20250108062834.11117-2-kuniyu@amazon.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series gtp/pfcp: Fix use-after-free of UDP tunnel socket. | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1 this patch: 2
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: osmocom-net-gprs@lists.osmocom.org
netdev/build_clang fail Errors and warnings before: 2 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 1 this patch: 2
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 45 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kuniyuki Iwashima Jan. 8, 2025, 6:28 a.m. UTC 
gtp_newlink() links the gtp device to a list in dev_net(dev).

However, even after the gtp device is moved to another netns,
it stays on the list but should be invisible.

Let's use for_each_netdev_rcu() for netdev traversal in
gtp_genl_dump_pdp().

Note that gtp_dev_list is no longer used under RCU, so list
helpers are converted to the non-RCU variant.

Fixes: 459aa660eb1d ("gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)")
Reported-by: Xiao Liang <shaw.leon@gmail.com>
Closes: https://lore.kernel.org/netdev/CABAhCOQdBL6h9M2C+kd+bGivRJ9Q72JUxW+-gur0nub_=PmFPA@mail.gmail.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Harald Welte <laforge@gnumonks.org>
---
 drivers/net/gtp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Simon Horman Jan. 8, 2025, 10:40 a.m. UTC | #1 
On Wed, Jan 08, 2025 at 03:28:32PM +0900, Kuniyuki Iwashima wrote:
> gtp_newlink() links the gtp device to a list in dev_net(dev).
> 
> However, even after the gtp device is moved to another netns,
> it stays on the list but should be invisible.
> 
> Let's use for_each_netdev_rcu() for netdev traversal in
> gtp_genl_dump_pdp().
> 
> Note that gtp_dev_list is no longer used under RCU, so list
> helpers are converted to the non-RCU variant.
> 
> Fixes: 459aa660eb1d ("gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)")
> Reported-by: Xiao Liang <shaw.leon@gmail.com>
> Closes: https://lore.kernel.org/netdev/CABAhCOQdBL6h9M2C+kd+bGivRJ9Q72JUxW+-gur0nub_=PmFPA@mail.gmail.com/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Harald Welte <laforge@gnumonks.org>

...

> @@ -2280,7 +2281,10 @@ static int gtp_genl_dump_pdp(struct sk_buff *skb,
>  		return 0;
>  
>  	rcu_read_lock();
> -	list_for_each_entry_rcu(gtp, &gn->gtp_dev_list, list) {
> +	for_each_netdev_rcu(net, dev) {
> +		if (dev->rtnl_link_ops != &gtp_link_ops)
> +			continue;
> +
>  		if (last_gtp && last_gtp != gtp)
>  			continue;
>  		else

Hi Iwashima-san,

With this change gtp seems to be uninitialised here.
And, also, it looks like gn is now set but otherwise unused in this function.

Flagged by W=1 builds with clang-19.

...
Kuniyuki Iwashima Jan. 8, 2025, 12:25 p.m. UTC | #2 
From: Simon Horman <horms@kernel.org>
Date: Wed, 8 Jan 2025 10:40:40 +0000
> On Wed, Jan 08, 2025 at 03:28:32PM +0900, Kuniyuki Iwashima wrote:
> > gtp_newlink() links the gtp device to a list in dev_net(dev).
> > 
> > However, even after the gtp device is moved to another netns,
> > it stays on the list but should be invisible.
> > 
> > Let's use for_each_netdev_rcu() for netdev traversal in
> > gtp_genl_dump_pdp().
> > 
> > Note that gtp_dev_list is no longer used under RCU, so list
> > helpers are converted to the non-RCU variant.
> > 
> > Fixes: 459aa660eb1d ("gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)")
> > Reported-by: Xiao Liang <shaw.leon@gmail.com>
> > Closes: https://lore.kernel.org/netdev/CABAhCOQdBL6h9M2C+kd+bGivRJ9Q72JUxW+-gur0nub_=PmFPA@mail.gmail.com/
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > Cc: Harald Welte <laforge@gnumonks.org>
> 
> ...
> 
> > @@ -2280,7 +2281,10 @@ static int gtp_genl_dump_pdp(struct sk_buff *skb,
> >  		return 0;
> >  
> >  	rcu_read_lock();
> > -	list_for_each_entry_rcu(gtp, &gn->gtp_dev_list, list) {
> > +	for_each_netdev_rcu(net, dev) {
> > +		if (dev->rtnl_link_ops != &gtp_link_ops)
> > +			continue;
> > +
> >  		if (last_gtp && last_gtp != gtp)
> >  			continue;
> >  		else
> 
> Hi Iwashima-san,
> 
> With this change gtp seems to be uninitialised here.
> And, also, it looks like gn is now set but otherwise unused in this function.
> 
> Flagged by W=1 builds with clang-19.

Ah, thank you for catching these !

I'll squash this to v2.

---8<---
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index b905b7bc46cb..0d03e150efc6 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -2298,9 +2298,6 @@ static int gtp_genl_dump_pdp(struct sk_buff *skb,
 	struct net *net = sock_net(skb->sk);
 	struct net_device *dev;
 	struct pdp_ctx *pctx;
-	struct gtp_net *gn;
-
-	gn = net_generic(net, gtp_net_id);
 
 	if (cb->args[4])
 		return 0;
@@ -2310,6 +2307,8 @@ static int gtp_genl_dump_pdp(struct sk_buff *skb,
 		if (dev->rtnl_link_ops != &gtp_link_ops)
 			continue;
 
+		gtp = netdev_priv(dev);
+
 		if (last_gtp && last_gtp != gtp)
 			continue;
 		else
---8<---
kernel test robot Jan. 9, 2025, 1:59 a.m. UTC | #3 
Hi Kuniyuki,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/gtp-Use-for_each_netdev_rcu-in-gtp_genl_dump_pdp/20250108-143107
base:   net/main
patch link:    https://lore.kernel.org/r/20250108062834.11117-2-kuniyu%40amazon.com
patch subject: [PATCH v1 net 1/3] gtp: Use for_each_netdev_rcu() in gtp_genl_dump_pdp().
config: powerpc-randconfig-r073-20250109 (https://download.01.org/0day-ci/archive/20250109/202501090934.2wvM0EAi-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 096551537b2a747a3387726ca618ceeb3950e9bc)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501090934.2wvM0EAi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501090934.2wvM0EAi-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/gtp.c:2276:18: warning: variable 'gn' set but not used [-Wunused-but-set-variable]
    2276 |         struct gtp_net *gn;
         |                         ^
   drivers/net/gtp.c:2288:31: warning: variable 'gtp' is uninitialized when used here [-Wuninitialized]
    2288 |                 if (last_gtp && last_gtp != gtp)
         |                                             ^~~
   drivers/net/gtp.c:2271:64: note: initialize the variable 'gtp' to silence this warning
    2271 |         struct gtp_dev *last_gtp = (struct gtp_dev *)cb->args[2], *gtp;
         |                                                                       ^
         |                                                                        = NULL
   2 warnings generated.


vim +/gn +2276 drivers/net/gtp.c

459aa660eb1d8c Pablo Neira        2016-05-09  2267  
459aa660eb1d8c Pablo Neira        2016-05-09  2268  static int gtp_genl_dump_pdp(struct sk_buff *skb,
459aa660eb1d8c Pablo Neira        2016-05-09  2269  				struct netlink_callback *cb)
459aa660eb1d8c Pablo Neira        2016-05-09  2270  {
459aa660eb1d8c Pablo Neira        2016-05-09  2271  	struct gtp_dev *last_gtp = (struct gtp_dev *)cb->args[2], *gtp;
94a6d9fb88df43 Taehee Yoo         2019-12-11  2272  	int i, j, bucket = cb->args[0], skip = cb->args[1];
459aa660eb1d8c Pablo Neira        2016-05-09  2273  	struct net *net = sock_net(skb->sk);
766a8e9a311068 Kuniyuki Iwashima  2025-01-08  2274  	struct net_device *dev;
459aa660eb1d8c Pablo Neira        2016-05-09  2275  	struct pdp_ctx *pctx;
94a6d9fb88df43 Taehee Yoo         2019-12-11 @2276  	struct gtp_net *gn;
94a6d9fb88df43 Taehee Yoo         2019-12-11  2277  
94a6d9fb88df43 Taehee Yoo         2019-12-11  2278  	gn = net_generic(net, gtp_net_id);
459aa660eb1d8c Pablo Neira        2016-05-09  2279  
459aa660eb1d8c Pablo Neira        2016-05-09  2280  	if (cb->args[4])
459aa660eb1d8c Pablo Neira        2016-05-09  2281  		return 0;
459aa660eb1d8c Pablo Neira        2016-05-09  2282  
94a6d9fb88df43 Taehee Yoo         2019-12-11  2283  	rcu_read_lock();
766a8e9a311068 Kuniyuki Iwashima  2025-01-08  2284  	for_each_netdev_rcu(net, dev) {
766a8e9a311068 Kuniyuki Iwashima  2025-01-08  2285  		if (dev->rtnl_link_ops != &gtp_link_ops)
766a8e9a311068 Kuniyuki Iwashima  2025-01-08  2286  			continue;
766a8e9a311068 Kuniyuki Iwashima  2025-01-08  2287  
459aa660eb1d8c Pablo Neira        2016-05-09  2288  		if (last_gtp && last_gtp != gtp)
459aa660eb1d8c Pablo Neira        2016-05-09  2289  			continue;
459aa660eb1d8c Pablo Neira        2016-05-09  2290  		else
459aa660eb1d8c Pablo Neira        2016-05-09  2291  			last_gtp = NULL;
459aa660eb1d8c Pablo Neira        2016-05-09  2292  
94a6d9fb88df43 Taehee Yoo         2019-12-11  2293  		for (i = bucket; i < gtp->hash_size; i++) {
94a6d9fb88df43 Taehee Yoo         2019-12-11  2294  			j = 0;
94a6d9fb88df43 Taehee Yoo         2019-12-11  2295  			hlist_for_each_entry_rcu(pctx, &gtp->tid_hash[i],
94a6d9fb88df43 Taehee Yoo         2019-12-11  2296  						 hlist_tid) {
94a6d9fb88df43 Taehee Yoo         2019-12-11  2297  				if (j >= skip &&
94a6d9fb88df43 Taehee Yoo         2019-12-11  2298  				    gtp_genl_fill_info(skb,
459aa660eb1d8c Pablo Neira        2016-05-09  2299  					    NETLINK_CB(cb->skb).portid,
459aa660eb1d8c Pablo Neira        2016-05-09  2300  					    cb->nlh->nlmsg_seq,
846c68f7f1ac82 Yoshiyuki Kurauchi 2020-04-30  2301  					    NLM_F_MULTI,
94a6d9fb88df43 Taehee Yoo         2019-12-11  2302  					    cb->nlh->nlmsg_type, pctx)) {
459aa660eb1d8c Pablo Neira        2016-05-09  2303  					cb->args[0] = i;
94a6d9fb88df43 Taehee Yoo         2019-12-11  2304  					cb->args[1] = j;
459aa660eb1d8c Pablo Neira        2016-05-09  2305  					cb->args[2] = (unsigned long)gtp;
459aa660eb1d8c Pablo Neira        2016-05-09  2306  					goto out;
459aa660eb1d8c Pablo Neira        2016-05-09  2307  				}
94a6d9fb88df43 Taehee Yoo         2019-12-11  2308  				j++;
459aa660eb1d8c Pablo Neira        2016-05-09  2309  			}
94a6d9fb88df43 Taehee Yoo         2019-12-11  2310  			skip = 0;
459aa660eb1d8c Pablo Neira        2016-05-09  2311  		}
94a6d9fb88df43 Taehee Yoo         2019-12-11  2312  		bucket = 0;
459aa660eb1d8c Pablo Neira        2016-05-09  2313  	}
459aa660eb1d8c Pablo Neira        2016-05-09  2314  	cb->args[4] = 1;
459aa660eb1d8c Pablo Neira        2016-05-09  2315  out:
94a6d9fb88df43 Taehee Yoo         2019-12-11  2316  	rcu_read_unlock();
459aa660eb1d8c Pablo Neira        2016-05-09  2317  	return skb->len;
459aa660eb1d8c Pablo Neira        2016-05-09  2318  }
459aa660eb1d8c Pablo Neira        2016-05-09  2319
diff mbox series

Patch

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 89a996ad8cd0..8e456d09d173 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1525,7 +1525,7 @@  static int gtp_newlink(struct net *src_net, struct net_device *dev,
 	}
 
 	gn = net_generic(dev_net(dev), gtp_net_id);
-	list_add_rcu(&gtp->list, &gn->gtp_dev_list);
+	list_add(&gtp->list, &gn->gtp_dev_list);
 	dev->priv_destructor = gtp_destructor;
 
 	netdev_dbg(dev, "registered new GTP interface\n");
@@ -1551,7 +1551,7 @@  static void gtp_dellink(struct net_device *dev, struct list_head *head)
 		hlist_for_each_entry_safe(pctx, next, &gtp->tid_hash[i], hlist_tid)
 			pdp_context_delete(pctx);
 
-	list_del_rcu(&gtp->list);
+	list_del(&gtp->list);
 	unregister_netdevice_queue(dev, head);
 }
 
@@ -2271,6 +2271,7 @@  static int gtp_genl_dump_pdp(struct sk_buff *skb,
 	struct gtp_dev *last_gtp = (struct gtp_dev *)cb->args[2], *gtp;
 	int i, j, bucket = cb->args[0], skip = cb->args[1];
 	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
 	struct pdp_ctx *pctx;
 	struct gtp_net *gn;
 
@@ -2280,7 +2281,10 @@  static int gtp_genl_dump_pdp(struct sk_buff *skb,
 		return 0;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(gtp, &gn->gtp_dev_list, list) {
+	for_each_netdev_rcu(net, dev) {
+		if (dev->rtnl_link_ops != &gtp_link_ops)
+			continue;
+
 		if (last_gtp && last_gtp != gtp)
 			continue;
 		else
@@ -2475,9 +2479,9 @@  static void __net_exit gtp_net_exit_batch_rtnl(struct list_head *net_list,
 
 	list_for_each_entry(net, net_list, exit_list) {
 		struct gtp_net *gn = net_generic(net, gtp_net_id);
-		struct gtp_dev *gtp;
+		struct gtp_dev *gtp, *gtp_next;
 
-		list_for_each_entry(gtp, &gn->gtp_dev_list, list)
+		list_for_each_entry_safe(gtp, gtp_next, &gn->gtp_dev_list, list)
 			gtp_dellink(gtp->dev, dev_to_kill);
 	}
 }