| Message ID | 20221129055317.53788-1-duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: Add a gfp_t parameter in ip_fib_metrics_init to support atomic context | expand |
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: > > 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. > > 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; I understand it, thank your for your advice. Best regards, Duoming Zhou
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. */
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 <duoming@zju.edu.cn> --- 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(-)