Skip to content

Commit c3e8d5a

Browse files
kuba-mookuba-moo
committed
Merge branch 'net-smc-two-fixes-for-smc-fallback'
Wen Gu says: ==================== net/smc: Two fixes for smc fallback This patch set includes two fixes for smc fallback: Patch 1/2 introduces some simple helpers to wrap the replacement and restore of clcsock's callback functions. Make sure that only the original callbacks will be saved and not overwritten. Patch 2/2 fixes a syzbot reporting slab-out-of-bound issue where smc_fback_error_report() accesses the already freed smc sock (see https://lore.kernel.org/r/[email protected]/). The patch fixes it by resetting sk_user_data and restoring clcsock callback functions timely in fallback situation. But it should be noted that although patch 2/2 can fix the issue of 'slab-out-of-bounds/use-after-free in smc_fback_error_report', it can't pass the syzbot reproducer test. Because after applying these two patches in upstream, syzbot reproducer triggered another known issue like this: ================================================================== BUG: KASAN: use-after-free in tcp_retransmit_timer+0x2ef3/0x3360 net/ipv4/tcp_timer.c:511 Read of size 8 at addr ffff888020328380 by task udevd/4158 CPU: 1 PID: 4158 Comm: udevd Not tainted 5.18.0-rc3-syzkaller-00074-gb05a5683eba6-dirty #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: <IRQ> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 print_address_description.constprop.0.cold+0xeb/0x467 mm/kasan/report.c:313 print_report mm/kasan/report.c:429 [inline] kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491 tcp_retransmit_timer+0x2ef3/0x3360 net/ipv4/tcp_timer.c:511 tcp_write_timer_handler+0x5e6/0xbc0 net/ipv4/tcp_timer.c:622 tcp_write_timer+0xa2/0x2b0 net/ipv4/tcp_timer.c:642 call_timer_fn+0x1a5/0x6b0 kernel/time/timer.c:1421 expire_timers kernel/time/timer.c:1466 [inline] __run_timers.part.0+0x679/0xa80 kernel/time/timer.c:1737 __run_timers kernel/time/timer.c:1715 [inline] run_timer_softirq+0xb3/0x1d0 kernel/time/timer.c:1750 __do_softirq+0x29b/0x9c2 kernel/softirq.c:558 invoke_softirq kernel/softirq.c:432 [inline] __irq_exit_rcu+0x123/0x180 kernel/softirq.c:637 irq_exit_rcu+0x5/0x20 kernel/softirq.c:649 sysvec_apic_timer_interrupt+0x93/0xc0 arch/x86/kernel/apic/apic.c:1097 </IRQ> ... (detail report can be found in https://syzkaller.appspot.com/text?tag=CrashReport&x=15406b44f00000) IMHO, the above issue is the same as this known one: https://syzkaller.appspot.com/bug?extid=694120e1002c117747ed, and it doesn't seem to be related with SMC. The discussion about this known issue is ongoing and can be found in https://lore.kernel.org/bpf/[email protected]/T/. And I added the temporary solution mentioned in the above discussion on top of my two patches, the syzbot reproducer of 'slab-out-of-bounds/ use-after-free in smc_fback_error_report' no longer triggers any issue. ==================== Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>
2 parents ba5a4fd + 0558226 commit c3e8d5a

File tree

3 files changed

+126
-43
lines changed

3 files changed

+126
-43
lines changed

‎net/smc/af_smc.c‎

Lines changed: 93 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -243,11 +243,27 @@ struct proto smc_proto6 = {
243243
};
244244
EXPORT_SYMBOL_GPL(smc_proto6);
245245

246+
static void smc_fback_restore_callbacks(struct smc_sock *smc)
247+
{
248+
struct sock *clcsk = smc->clcsock->sk;
249+
250+
write_lock_bh(&clcsk->sk_callback_lock);
251+
clcsk->sk_user_data = NULL;
252+
253+
smc_clcsock_restore_cb(&clcsk->sk_state_change, &smc->clcsk_state_change);
254+
smc_clcsock_restore_cb(&clcsk->sk_data_ready, &smc->clcsk_data_ready);
255+
smc_clcsock_restore_cb(&clcsk->sk_write_space, &smc->clcsk_write_space);
256+
smc_clcsock_restore_cb(&clcsk->sk_error_report, &smc->clcsk_error_report);
257+
258+
write_unlock_bh(&clcsk->sk_callback_lock);
259+
}
260+
246261
static void smc_restore_fallback_changes(struct smc_sock *smc)
247262
{
248263
if (smc->clcsock->file) { /* non-accepted sockets have no file yet */
249264
smc->clcsock->file->private_data = smc->sk.sk_socket;
250265
smc->clcsock->file = NULL;
266+
smc_fback_restore_callbacks(smc);
251267
}
252268
}
253269

@@ -373,6 +389,7 @@ static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
373389
sk->sk_prot->hash(sk);
374390
sk_refcnt_debug_inc(sk);
375391
mutex_init(&smc->clcsock_release_lock);
392+
smc_init_saved_callbacks(smc);
376393

377394
return sk;
378395
}
@@ -744,58 +761,81 @@ static void smc_fback_forward_wakeup(struct smc_sock *smc, struct sock *clcsk,
744761

745762
static void smc_fback_state_change(struct sock *clcsk)
746763
{
747-
struct smc_sock *smc =
748-
smc_clcsock_user_data(clcsk);
764+
struct smc_sock *smc;
749765

750-
if (!smc)
751-
return;
752-
smc_fback_forward_wakeup(smc, clcsk, smc->clcsk_state_change);
766+
read_lock_bh(&clcsk->sk_callback_lock);
767+
smc = smc_clcsock_user_data(clcsk);
768+
if (smc)
769+
smc_fback_forward_wakeup(smc, clcsk,
770+
smc->clcsk_state_change);
771+
read_unlock_bh(&clcsk->sk_callback_lock);
753772
}
754773

755774
static void smc_fback_data_ready(struct sock *clcsk)
756775
{
757-
struct smc_sock *smc =
758-
smc_clcsock_user_data(clcsk);
776+
struct smc_sock *smc;
759777

760-
if (!smc)
761-
return;
762-
smc_fback_forward_wakeup(smc, clcsk, smc->clcsk_data_ready);
778+
read_lock_bh(&clcsk->sk_callback_lock);
779+
smc = smc_clcsock_user_data(clcsk);
780+
if (smc)
781+
smc_fback_forward_wakeup(smc, clcsk,
782+
smc->clcsk_data_ready);
783+
read_unlock_bh(&clcsk->sk_callback_lock);
763784
}
764785

765786
static void smc_fback_write_space(struct sock *clcsk)
766787
{
767-
struct smc_sock *smc =
768-
smc_clcsock_user_data(clcsk);
788+
struct smc_sock *smc;
769789

770-
if (!smc)
771-
return;
772-
smc_fback_forward_wakeup(smc, clcsk, smc->clcsk_write_space);
790+
read_lock_bh(&clcsk->sk_callback_lock);
791+
smc = smc_clcsock_user_data(clcsk);
792+
if (smc)
793+
smc_fback_forward_wakeup(smc, clcsk,
794+
smc->clcsk_write_space);
795+
read_unlock_bh(&clcsk->sk_callback_lock);
773796
}
774797

775798
static void smc_fback_error_report(struct sock *clcsk)
776799
{
777-
struct smc_sock *smc =
778-
smc_clcsock_user_data(clcsk);
800+
struct smc_sock *smc;
779801

780-
if (!smc)
781-
return;
782-
smc_fback_forward_wakeup(smc, clcsk, smc->clcsk_error_report);
802+
read_lock_bh(&clcsk->sk_callback_lock);
803+
smc = smc_clcsock_user_data(clcsk);
804+
if (smc)
805+
smc_fback_forward_wakeup(smc, clcsk,
806+
smc->clcsk_error_report);
807+
read_unlock_bh(&clcsk->sk_callback_lock);
808+
}
809+
810+
static void smc_fback_replace_callbacks(struct smc_sock *smc)
811+
{
812+
struct sock *clcsk = smc->clcsock->sk;
813+
814+
write_lock_bh(&clcsk->sk_callback_lock);
815+
clcsk->sk_user_data = (void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
816+
817+
smc_clcsock_replace_cb(&clcsk->sk_state_change, smc_fback_state_change,
818+
&smc->clcsk_state_change);
819+
smc_clcsock_replace_cb(&clcsk->sk_data_ready, smc_fback_data_ready,
820+
&smc->clcsk_data_ready);
821+
smc_clcsock_replace_cb(&clcsk->sk_write_space, smc_fback_write_space,
822+
&smc->clcsk_write_space);
823+
smc_clcsock_replace_cb(&clcsk->sk_error_report, smc_fback_error_report,
824+
&smc->clcsk_error_report);
825+
826+
write_unlock_bh(&clcsk->sk_callback_lock);
783827
}
784828

785829
static int smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
786830
{
787-
struct sock *clcsk;
788831
int rc = 0;
789832

790833
mutex_lock(&smc->clcsock_release_lock);
791834
if (!smc->clcsock) {
792835
rc = -EBADF;
793836
goto out;
794837
}
795-
clcsk = smc->clcsock->sk;
796838

797-
if (smc->use_fallback)
798-
goto out;
799839
smc->use_fallback = true;
800840
smc->fallback_rsn = reason_code;
801841
smc_stat_fallback(smc);
@@ -810,18 +850,7 @@ static int smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
810850
* in smc sk->sk_wq and they should be woken up
811851
* as clcsock's wait queue is woken up.
812852
*/
813-
smc->clcsk_state_change = clcsk->sk_state_change;
814-
smc->clcsk_data_ready = clcsk->sk_data_ready;
815-
smc->clcsk_write_space = clcsk->sk_write_space;
816-
smc->clcsk_error_report = clcsk->sk_error_report;
817-
818-
clcsk->sk_state_change = smc_fback_state_change;
819-
clcsk->sk_data_ready = smc_fback_data_ready;
820-
clcsk->sk_write_space = smc_fback_write_space;
821-
clcsk->sk_error_report = smc_fback_error_report;
822-
823-
smc->clcsock->sk->sk_user_data =
824-
(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
853+
smc_fback_replace_callbacks(smc);
825854
}
826855
out:
827856
mutex_unlock(&smc->clcsock_release_lock);
@@ -1596,6 +1625,19 @@ static int smc_clcsock_accept(struct smc_sock *lsmc, struct smc_sock **new_smc)
15961625
* function; switch it back to the original sk_data_ready function
15971626
*/
15981627
new_clcsock->sk->sk_data_ready = lsmc->clcsk_data_ready;
1628+
1629+
/* if new clcsock has also inherited the fallback-specific callback
1630+
* functions, switch them back to the original ones.
1631+
*/
1632+
if (lsmc->use_fallback) {
1633+
if (lsmc->clcsk_state_change)
1634+
new_clcsock->sk->sk_state_change = lsmc->clcsk_state_change;
1635+
if (lsmc->clcsk_write_space)
1636+
new_clcsock->sk->sk_write_space = lsmc->clcsk_write_space;
1637+
if (lsmc->clcsk_error_report)
1638+
new_clcsock->sk->sk_error_report = lsmc->clcsk_error_report;
1639+
}
1640+
15991641
(*new_smc)->clcsock = new_clcsock;
16001642
out:
16011643
return rc;
@@ -2355,17 +2397,20 @@ static void smc_tcp_listen_work(struct work_struct *work)
23552397

23562398
static void smc_clcsock_data_ready(struct sock *listen_clcsock)
23572399
{
2358-
struct smc_sock *lsmc =
2359-
smc_clcsock_user_data(listen_clcsock);
2400+
struct smc_sock *lsmc;
23602401

2402+
read_lock_bh(&listen_clcsock->sk_callback_lock);
2403+
lsmc = smc_clcsock_user_data(listen_clcsock);
23612404
if (!lsmc)
2362-
return;
2405+
goto out;
23632406
lsmc->clcsk_data_ready(listen_clcsock);
23642407
if (lsmc->sk.sk_state == SMC_LISTEN) {
23652408
sock_hold(&lsmc->sk); /* sock_put in smc_tcp_listen_work() */
23662409
if (!queue_work(smc_tcp_ls_wq, &lsmc->tcp_listen_work))
23672410
sock_put(&lsmc->sk);
23682411
}
2412+
out:
2413+
read_unlock_bh(&listen_clcsock->sk_callback_lock);
23692414
}
23702415

23712416
static int smc_listen(struct socket *sock, int backlog)
@@ -2397,10 +2442,12 @@ static int smc_listen(struct socket *sock, int backlog)
23972442
/* save original sk_data_ready function and establish
23982443
* smc-specific sk_data_ready function
23992444
*/
2400-
smc->clcsk_data_ready = smc->clcsock->sk->sk_data_ready;
2401-
smc->clcsock->sk->sk_data_ready = smc_clcsock_data_ready;
2445+
write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
24022446
smc->clcsock->sk->sk_user_data =
24032447
(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
2448+
smc_clcsock_replace_cb(&smc->clcsock->sk->sk_data_ready,
2449+
smc_clcsock_data_ready, &smc->clcsk_data_ready);
2450+
write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
24042451

24052452
/* save original ops */
24062453
smc->ori_af_ops = inet_csk(smc->clcsock->sk)->icsk_af_ops;
@@ -2415,7 +2462,11 @@ static int smc_listen(struct socket *sock, int backlog)
24152462

24162463
rc = kernel_listen(smc->clcsock, backlog);
24172464
if (rc) {
2418-
smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;
2465+
write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
2466+
smc_clcsock_restore_cb(&smc->clcsock->sk->sk_data_ready,
2467+
&smc->clcsk_data_ready);
2468+
smc->clcsock->sk->sk_user_data = NULL;
2469+
write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
24192470
goto out;
24202471
}
24212472
sk->sk_max_ack_backlog = backlog;

‎net/smc/smc.h‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,12 +288,41 @@ static inline struct smc_sock *smc_sk(const struct sock *sk)
288288
return (struct smc_sock *)sk;
289289
}
290290

291+
static inline void smc_init_saved_callbacks(struct smc_sock *smc)
292+
{
293+
smc->clcsk_state_change = NULL;
294+
smc->clcsk_data_ready = NULL;
295+
smc->clcsk_write_space = NULL;
296+
smc->clcsk_error_report = NULL;
297+
}
298+
291299
static inline struct smc_sock *smc_clcsock_user_data(const struct sock *clcsk)
292300
{
293301
return (struct smc_sock *)
294302
((uintptr_t)clcsk->sk_user_data & ~SK_USER_DATA_NOCOPY);
295303
}
296304

305+
/* save target_cb in saved_cb, and replace target_cb with new_cb */
306+
static inline void smc_clcsock_replace_cb(void (**target_cb)(struct sock *),
307+
void (*new_cb)(struct sock *),
308+
void (**saved_cb)(struct sock *))
309+
{
310+
/* only save once */
311+
if (!*saved_cb)
312+
*saved_cb = *target_cb;
313+
*target_cb = new_cb;
314+
}
315+
316+
/* restore target_cb to saved_cb, and reset saved_cb to NULL */
317+
static inline void smc_clcsock_restore_cb(void (**target_cb)(struct sock *),
318+
void (**saved_cb)(struct sock *))
319+
{
320+
if (!*saved_cb)
321+
return;
322+
*target_cb = *saved_cb;
323+
*saved_cb = NULL;
324+
}
325+
297326
extern struct workqueue_struct *smc_hs_wq; /* wq for handshake work */
298327
extern struct workqueue_struct *smc_close_wq; /* wq for close work */
299328

‎net/smc/smc_close.c‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,11 @@ int smc_close_active(struct smc_sock *smc)
214214
sk->sk_state = SMC_CLOSED;
215215
sk->sk_state_change(sk); /* wake up accept */
216216
if (smc->clcsock && smc->clcsock->sk) {
217-
smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;
217+
write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
218+
smc_clcsock_restore_cb(&smc->clcsock->sk->sk_data_ready,
219+
&smc->clcsk_data_ready);
218220
smc->clcsock->sk->sk_user_data = NULL;
221+
write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
219222
rc = kernel_sock_shutdown(smc->clcsock, SHUT_RDWR);
220223
}
221224
smc_close_cleanup_listen(sk);

0 commit comments

Comments
 (0)