On Thu, Oct 13, 2022 at 10:39:08PM +0200, Jakub Sitnicki wrote:
> Hi Stan,
>
> On Wed, Oct 12, 2022 at 02:08 PM -07, sdf@xxxxxxxxxx wrote:
> > Hi John & Jakub,
> >
> > Upstream commit c0feea594e05 ("workqueue: don't skip lockdep work
> > dependency in cancel_work_sync()") seems to trigger the following
> > lockdep warning during test_prog's sockmap_listen:
> >
> > [ +0.003631] WARNING: possible circular locking dependency detected
>
> [...]
>
> > Are you ware? Any idea what's wrong?
> > Is there some stable fix I'm missing in bpf-next?
>
> Thanks for bringing it up. I didn't know.
>
> The mentioned commit doesn't look that fresh
>
> commit c0feea594e058223973db94c1c32a830c9807c86
> Author: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Date: Fri Jul 29 13:30:23 2022 +0900
>
> workqueue: don't skip lockdep work dependency in cancel_work_sync()
>
> ... but then it just landed not so long ago, which explains things:
>
> $ git describe --contains c0feea594e058223973db94c1c32a830c9807c86 --match 'v*'
> v6.0-rc7~10^2
>
> I've untangled the call chains leading to the potential dead-lock a
> bit. There does seem to be a window of opportunity there.
>
> psock->work.func = sk_psock_backlog()
> ACQUIRE psock->work_mutex
> sk_psock_handle_skb()
> skb_send_sock()
> __skb_send_sock()
> sendpage_unlocked()
> kernel_sendpage()
> sock->ops->sendpage = inet_sendpage()
> sk->sk_prot->sendpage = tcp_sendpage()
> ACQUIRE sk->sk_lock
> tcp_sendpage_locked()
> RELEASE sk->sk_lock
> RELEASE psock->work_mutex
>
> sock_map_close()
> ACQUIRE sk->sk_lock
> sk_psock_stop()
> sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED)
> cancel_work_sync()
> __cancel_work_timer()
> __flush_work()
> // wait for psock->work to finish
> RELEASE sk->sk_lock
>
> There is no fix I know of. Need to think. Ideas welcome.
>
Thanks for the analysis.
I wonder if we can simply move this cancel_work_sync() out of sock
lock... Something like this:
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 48f4b645193b..70d6cb94e580 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -376,7 +376,7 @@ static inline void sk_psock_report_error(struct sk_psock *psock, int err)
}
struct sk_psock *sk_psock_init(struct sock *sk, int node);
-void sk_psock_stop(struct sk_psock *psock, bool wait);
+void sk_psock_stop(struct sk_psock *psock);
#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock);
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index ca70525621c7..ddc56660ce97 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -803,16 +803,13 @@ static void sk_psock_link_destroy(struct sk_psock *psock)
}
}
-void sk_psock_stop(struct sk_psock *psock, bool wait)
+void sk_psock_stop(struct sk_psock *psock)
{
spin_lock_bh(&psock->ingress_lock);
sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
sk_psock_cork_free(psock);
__sk_psock_zap_ingress(psock);
spin_unlock_bh(&psock->ingress_lock);
-
- if (wait)
- cancel_work_sync(&psock->work);
}
static void sk_psock_done_strp(struct sk_psock *psock);
@@ -850,7 +847,7 @@ void sk_psock_drop(struct sock *sk, struct sk_psock *psock)
sk_psock_stop_verdict(sk, psock);
write_unlock_bh(&sk->sk_callback_lock);
- sk_psock_stop(psock, false);
+ sk_psock_stop(psock);
INIT_RCU_WORK(&psock->rwork, sk_psock_destroy);
queue_rcu_work(system_wq, &psock->rwork);
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index a660baedd9e7..81beb16ab1eb 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1596,7 +1596,7 @@ void sock_map_destroy(struct sock *sk)
saved_destroy = psock->saved_destroy;
sock_map_remove_links(sk, psock);
rcu_read_unlock();
- sk_psock_stop(psock, false);
+ sk_psock_stop(psock);
sk_psock_put(sk, psock);
saved_destroy(sk);
}
@@ -1619,9 +1619,10 @@ void sock_map_close(struct sock *sk, long timeout)
saved_close = psock->saved_close;
sock_map_remove_links(sk, psock);
rcu_read_unlock();
- sk_psock_stop(psock, true);
- sk_psock_put(sk, psock);
+ sk_psock_stop(psock);
release_sock(sk);
+ cancel_work_sync(&psock->work);
+ sk_psock_put(sk, psock);
saved_close(sk, timeout);
}
EXPORT_SYMBOL_GPL(sock_map_close);
