[PATCH net] net: Add a gfp_t parameter in ip_fib_metrics_init to support atomic context

The ip_fib_metrics_init() do not support atomic context, because it
calls "kzalloc(..., GFP_KERNEL)". When ip_fib_metrics_init() is used
in atomic context, the sleep-in-atomic-context bug will happen.

For example, the neigh_proxy_process() is a timer handler that is
used to process the proxy request that is timeout. But it could call
ip_fib_metrics_init(). As a result, the can_block flag in ipv6_add_addr()
and the gfp_flags in addrconf_f6i_alloc() and ip6_route_info_create()
are useless. The process is shown below.

    (atomic context)
neigh_proxy_process()
  pndisc_redo()
    ndisc_recv_ns()
      addrconf_dad_failure()
        ipv6_add_addr(..., bool can_block)
          addrconf_f6i_alloc(..., gfp_t gfp_flags)
            ip6_route_info_create(..., gfp_t gfp_flags)
              ip_fib_metrics_init()
                kzalloc(..., GFP_KERNEL) //may sleep

This patch add a gfp_t parameter in ip_fib_metrics_init() in order to
support atomic context.

Fixes: f3d9832e56c4 ("ipv6: addrconf: cleanup locking in ipv6_add_addr")
Signed-off-by: Duoming Zhou <redacted>
---
 include/net/ip.h         | 2 +-
 net/ipv4/fib_semantics.c | 2 +-
 net/ipv4/metrics.c       | 4 ++--
 net/ipv6/route.c         | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 144bdfbb25a..1e99c9d6240 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h

@@ -489,7 +489,7 @@ static inline unsigned int ip_skb_dst_mtu(struct sock *sk,
 
 struct dst_metrics *ip_fib_metrics_init(struct net *net, struct nlattr *fc_mx,
 					int fc_mx_len,
-					struct netlink_ext_ack *extack);
+					struct netlink_ext_ack *extack, gfp_t gfp_flags);
 static inline void ip_fib_metrics_put(struct dst_metrics *fib_metrics)
 {
 	if (fib_metrics != &dst_default_metrics &&

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index f721c308248..4a0793e7d34 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -1450,7 +1450,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 	if (!fi)
 		goto failure;
 	fi->fib_metrics = ip_fib_metrics_init(fi->fib_net, cfg->fc_mx,
-					      cfg->fc_mx_len, extack);
+					      cfg->fc_mx_len, extack, GFP_KERNEL);
 	if (IS_ERR(fi->fib_metrics)) {
 		err = PTR_ERR(fi->fib_metrics);
 		kfree(fi);

diff --git a/net/ipv4/metrics.c b/net/ipv4/metrics.c
index 25ea6ac44db..b0342603a6d 100644
--- a/net/ipv4/metrics.c
+++ b/net/ipv4/metrics.c

@@ -66,7 +66,7 @@ static int ip_metrics_convert(struct net *net, struct nlattr *fc_mx,
 
 struct dst_metrics *ip_fib_metrics_init(struct net *net, struct nlattr *fc_mx,
 					int fc_mx_len,
-					struct netlink_ext_ack *extack)
+					struct netlink_ext_ack *extack, gfp_t gfp_flags)
 {
 	struct dst_metrics *fib_metrics;
 	int err;

@@ -74,7 +74,7 @@ struct dst_metrics *ip_fib_metrics_init(struct net *net, struct nlattr *fc_mx,
 	if (!fc_mx)
 		return (struct dst_metrics *)&dst_default_metrics;
 
-	fib_metrics = kzalloc(sizeof(*fib_metrics), GFP_KERNEL);
+	fib_metrics = kzalloc(sizeof(*fib_metrics), gfp_flags);
 	if (unlikely(!fib_metrics))
 		return ERR_PTR(-ENOMEM);
 

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 2f355f0ec32..2687e764a87 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -3751,7 +3751,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 		goto out;
 
 	rt->fib6_metrics = ip_fib_metrics_init(net, cfg->fc_mx, cfg->fc_mx_len,
-					       extack);
+					       extack, gfp_flags);
 	if (IS_ERR(rt->fib6_metrics)) {
 		err = PTR_ERR(rt->fib6_metrics);
 		/* Do not leave garbage there. */

-- 
2.17.1

On 11/28/22 10:53 PM, Duoming Zhou wrote:

The ip_fib_metrics_init() do not support atomic context, because it
calls "kzalloc(..., GFP_KERNEL)". When ip_fib_metrics_init() is used
in atomic context, the sleep-in-atomic-context bug will happen.

Did you actually hit this sleep-in-atomic-context bug or is it theory
based on code analysis?

For example, the neigh_proxy_process() is a timer handler that is
used to process the proxy request that is timeout. But it could call
ip_fib_metrics_init(). As a result, the can_block flag in ipv6_add_addr()
and the gfp_flags in addrconf_f6i_alloc() and ip6_route_info_create()
are useless. The process is shown below.

    (atomic context)
neigh_proxy_process()
  pndisc_redo()
    ndisc_recv_ns()
      addrconf_dad_failure()
        ipv6_add_addr(..., bool can_block)
          addrconf_f6i_alloc(..., gfp_t gfp_flags)

	cfg has fc_mx == NULL.

            ip6_route_info_create(..., gfp_t gfp_flags)

	rt->fib6_metrics = ip_fib_metrics_init(net, cfg->fc_mx, cfg->fc_mx_len,
extack);

              ip_fib_metrics_init()

        if (!fc_mx)
                return (struct dst_metrics *)&dst_default_metrics;

                kzalloc(..., GFP_KERNEL) //may sleep

Hello,

On Tue, 29 Nov 2022 09:33:45 -0700 David Ahern wrote:

On 11/28/22 10:53 PM, Duoming Zhou wrote:

quoted

The ip_fib_metrics_init() do not support atomic context, because it
calls "kzalloc(..., GFP_KERNEL)". When ip_fib_metrics_init() is used
in atomic context, the sleep-in-atomic-context bug will happen.

Did you actually hit this sleep-in-atomic-context bug or is it theory
based on code analysis?

Thank your for your reply and suggestions. This is based on code analysis.

quoted

For example, the neigh_proxy_process() is a timer handler that is
used to process the proxy request that is timeout. But it could call
ip_fib_metrics_init(). As a result, the can_block flag in ipv6_add_addr()
and the gfp_flags in addrconf_f6i_alloc() and ip6_route_info_create()
are useless. The process is shown below.

    (atomic context)
neigh_proxy_process()
  pndisc_redo()
    ndisc_recv_ns()
      addrconf_dad_failure()
        ipv6_add_addr(..., bool can_block)
          addrconf_f6i_alloc(..., gfp_t gfp_flags)

	cfg has fc_mx == NULL.

quoted

            ip6_route_info_create(..., gfp_t gfp_flags)

	rt->fib6_metrics = ip_fib_metrics_init(net, cfg->fc_mx, cfg->fc_mx_len,
extack);

quoted

              ip_fib_metrics_init()

        if (!fc_mx)
                return (struct dst_metrics *)&dst_default_metrics;

I understand it, thank your for your advice.

Best regards,
Duoming Zhou