Skip to content

Commit c3f4293

Browse files
author
Paolo Abeni
committed
Merge branch 'af_unix-fix-two-oob-issues'
Kuniyuki Iwashima says: ==================== af_unix: Fix two OOB issues. From: Kuniyuki Iwashima <[email protected]> Recently, two issues are reported regarding MSG_OOB. Patch 1 fixes issues that happen when multiple consumed OOB skbs are placed consecutively in the recv queue. Patch 2 fixes an inconsistent behaviour that close()ing a socket with a consumed OOB skb at the head of the recv queue triggers -ECONNRESET on the peer's recv(). v1: https://lore.kernel.org/netdev/[email protected]/ ==================== Link: https://patch.msgid.link/[email protected] Signed-off-by: Paolo Abeni <[email protected]>
2 parents 7544f3f + 632f55f commit c3f4293

File tree

2 files changed

+161
-12
lines changed

2 files changed

+161
-12
lines changed

‎net/unix/af_unix.c‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,11 @@ static void unix_sock_destructor(struct sock *sk)
660660
#endif
661661
}
662662

663+
static unsigned int unix_skb_len(const struct sk_buff *skb)
664+
{
665+
return skb->len - UNIXCB(skb).consumed;
666+
}
667+
663668
static void unix_release_sock(struct sock *sk, int embrion)
664669
{
665670
struct unix_sock *u = unix_sk(sk);
@@ -694,10 +699,16 @@ static void unix_release_sock(struct sock *sk, int embrion)
694699

695700
if (skpair != NULL) {
696701
if (sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) {
702+
struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
703+
704+
#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
705+
if (skb && !unix_skb_len(skb))
706+
skb = skb_peek_next(skb, &sk->sk_receive_queue);
707+
#endif
697708
unix_state_lock(skpair);
698709
/* No more writes */
699710
WRITE_ONCE(skpair->sk_shutdown, SHUTDOWN_MASK);
700-
if (!skb_queue_empty_lockless(&sk->sk_receive_queue) || embrion)
711+
if (skb || embrion)
701712
WRITE_ONCE(skpair->sk_err, ECONNRESET);
702713
unix_state_unlock(skpair);
703714
skpair->sk_state_change(skpair);
@@ -2661,11 +2672,6 @@ static long unix_stream_data_wait(struct sock *sk, long timeo,
26612672
return timeo;
26622673
}
26632674

2664-
static unsigned int unix_skb_len(const struct sk_buff *skb)
2665-
{
2666-
return skb->len - UNIXCB(skb).consumed;
2667-
}
2668-
26692675
struct unix_stream_read_state {
26702676
int (*recv_actor)(struct sk_buff *, int, int,
26712677
struct unix_stream_read_state *);
@@ -2680,11 +2686,11 @@ struct unix_stream_read_state {
26802686
#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
26812687
static int unix_stream_recv_urg(struct unix_stream_read_state *state)
26822688
{
2689+
struct sk_buff *oob_skb, *read_skb = NULL;
26832690
struct socket *sock = state->socket;
26842691
struct sock *sk = sock->sk;
26852692
struct unix_sock *u = unix_sk(sk);
26862693
int chunk = 1;
2687-
struct sk_buff *oob_skb;
26882694

26892695
mutex_lock(&u->iolock);
26902696
unix_state_lock(sk);
@@ -2699,9 +2705,16 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
26992705

27002706
oob_skb = u->oob_skb;
27012707

2702-
if (!(state->flags & MSG_PEEK))
2708+
if (!(state->flags & MSG_PEEK)) {
27032709
WRITE_ONCE(u->oob_skb, NULL);
27042710

2711+
if (oob_skb->prev != (struct sk_buff *)&sk->sk_receive_queue &&
2712+
!unix_skb_len(oob_skb->prev)) {
2713+
read_skb = oob_skb->prev;
2714+
__skb_unlink(read_skb, &sk->sk_receive_queue);
2715+
}
2716+
}
2717+
27052718
spin_unlock(&sk->sk_receive_queue.lock);
27062719
unix_state_unlock(sk);
27072720

@@ -2712,6 +2725,8 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
27122725

27132726
mutex_unlock(&u->iolock);
27142727

2728+
consume_skb(read_skb);
2729+
27152730
if (chunk < 0)
27162731
return -EFAULT;
27172732

‎tools/testing/selftests/net/af_unix/msg_oob.c‎

Lines changed: 138 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ static void __sendpair(struct __test_metadata *_metadata,
210210
static void __recvpair(struct __test_metadata *_metadata,
211211
FIXTURE_DATA(msg_oob) *self,
212212
const char *expected_buf, int expected_len,
213-
int buf_len, int flags)
213+
int buf_len, int flags, bool is_sender)
214214
{
215215
int i, ret[2], recv_errno[2], expected_errno = 0;
216216
char recv_buf[2][BUF_SZ] = {};
@@ -221,7 +221,9 @@ static void __recvpair(struct __test_metadata *_metadata,
221221
errno = 0;
222222

223223
for (i = 0; i < 2; i++) {
224-
ret[i] = recv(self->fd[i * 2 + 1], recv_buf[i], buf_len, flags);
224+
int index = is_sender ? i * 2 : i * 2 + 1;
225+
226+
ret[i] = recv(self->fd[index], recv_buf[i], buf_len, flags);
225227
recv_errno[i] = errno;
226228
}
227229

@@ -308,6 +310,20 @@ static void __siocatmarkpair(struct __test_metadata *_metadata,
308310
ASSERT_EQ(answ[0], answ[1]);
309311
}
310312

313+
static void __resetpair(struct __test_metadata *_metadata,
314+
FIXTURE_DATA(msg_oob) *self,
315+
const FIXTURE_VARIANT(msg_oob) *variant,
316+
bool reset)
317+
{
318+
int i;
319+
320+
for (i = 0; i < 2; i++)
321+
close(self->fd[i * 2 + 1]);
322+
323+
__recvpair(_metadata, self, "", reset ? -ECONNRESET : 0, 1,
324+
variant->peek ? MSG_PEEK : 0, true);
325+
}
326+
311327
#define sendpair(buf, len, flags) \
312328
__sendpair(_metadata, self, buf, len, flags)
313329

@@ -316,9 +332,10 @@ static void __siocatmarkpair(struct __test_metadata *_metadata,
316332
if (variant->peek) \
317333
__recvpair(_metadata, self, \
318334
expected_buf, expected_len, \
319-
buf_len, (flags) | MSG_PEEK); \
335+
buf_len, (flags) | MSG_PEEK, false); \
320336
__recvpair(_metadata, self, \
321-
expected_buf, expected_len, buf_len, flags); \
337+
expected_buf, expected_len, \
338+
buf_len, flags, false); \
322339
} while (0)
323340

324341
#define epollpair(oob_remaining) \
@@ -330,6 +347,9 @@ static void __siocatmarkpair(struct __test_metadata *_metadata,
330347
#define setinlinepair() \
331348
__setinlinepair(_metadata, self)
332349

350+
#define resetpair(reset) \
351+
__resetpair(_metadata, self, variant, reset)
352+
333353
#define tcp_incompliant \
334354
for (self->tcp_compliant = false; \
335355
self->tcp_compliant == false; \
@@ -344,6 +364,21 @@ TEST_F(msg_oob, non_oob)
344364
recvpair("", -EINVAL, 1, MSG_OOB);
345365
epollpair(false);
346366
siocatmarkpair(false);
367+
368+
resetpair(true);
369+
}
370+
371+
TEST_F(msg_oob, non_oob_no_reset)
372+
{
373+
sendpair("x", 1, 0);
374+
epollpair(false);
375+
siocatmarkpair(false);
376+
377+
recvpair("x", 1, 1, 0);
378+
epollpair(false);
379+
siocatmarkpair(false);
380+
381+
resetpair(false);
347382
}
348383

349384
TEST_F(msg_oob, oob)
@@ -355,6 +390,19 @@ TEST_F(msg_oob, oob)
355390
recvpair("x", 1, 1, MSG_OOB);
356391
epollpair(false);
357392
siocatmarkpair(true);
393+
394+
tcp_incompliant {
395+
resetpair(false); /* TCP sets -ECONNRESET for ex-OOB. */
396+
}
397+
}
398+
399+
TEST_F(msg_oob, oob_reset)
400+
{
401+
sendpair("x", 1, MSG_OOB);
402+
epollpair(true);
403+
siocatmarkpair(true);
404+
405+
resetpair(true);
358406
}
359407

360408
TEST_F(msg_oob, oob_drop)
@@ -370,6 +418,8 @@ TEST_F(msg_oob, oob_drop)
370418
recvpair("", -EINVAL, 1, MSG_OOB);
371419
epollpair(false);
372420
siocatmarkpair(false);
421+
422+
resetpair(false);
373423
}
374424

375425
TEST_F(msg_oob, oob_ahead)
@@ -385,6 +435,10 @@ TEST_F(msg_oob, oob_ahead)
385435
recvpair("hell", 4, 4, 0);
386436
epollpair(false);
387437
siocatmarkpair(true);
438+
439+
tcp_incompliant {
440+
resetpair(false); /* TCP sets -ECONNRESET for ex-OOB. */
441+
}
388442
}
389443

390444
TEST_F(msg_oob, oob_break)
@@ -403,6 +457,8 @@ TEST_F(msg_oob, oob_break)
403457

404458
recvpair("", -EAGAIN, 1, 0);
405459
siocatmarkpair(false);
460+
461+
resetpair(false);
406462
}
407463

408464
TEST_F(msg_oob, oob_ahead_break)
@@ -426,6 +482,8 @@ TEST_F(msg_oob, oob_ahead_break)
426482
recvpair("world", 5, 5, 0);
427483
epollpair(false);
428484
siocatmarkpair(false);
485+
486+
resetpair(false);
429487
}
430488

431489
TEST_F(msg_oob, oob_break_drop)
@@ -449,6 +507,8 @@ TEST_F(msg_oob, oob_break_drop)
449507
recvpair("", -EINVAL, 1, MSG_OOB);
450508
epollpair(false);
451509
siocatmarkpair(false);
510+
511+
resetpair(false);
452512
}
453513

454514
TEST_F(msg_oob, ex_oob_break)
@@ -476,6 +536,8 @@ TEST_F(msg_oob, ex_oob_break)
476536
recvpair("ld", 2, 2, 0);
477537
epollpair(false);
478538
siocatmarkpair(false);
539+
540+
resetpair(false);
479541
}
480542

481543
TEST_F(msg_oob, ex_oob_drop)
@@ -498,6 +560,8 @@ TEST_F(msg_oob, ex_oob_drop)
498560
epollpair(false);
499561
siocatmarkpair(true);
500562
}
563+
564+
resetpair(false);
501565
}
502566

503567
TEST_F(msg_oob, ex_oob_drop_2)
@@ -523,6 +587,8 @@ TEST_F(msg_oob, ex_oob_drop_2)
523587
epollpair(false);
524588
siocatmarkpair(true);
525589
}
590+
591+
resetpair(false);
526592
}
527593

528594
TEST_F(msg_oob, ex_oob_oob)
@@ -546,6 +612,54 @@ TEST_F(msg_oob, ex_oob_oob)
546612
recvpair("", -EINVAL, 1, MSG_OOB);
547613
epollpair(false);
548614
siocatmarkpair(false);
615+
616+
resetpair(false);
617+
}
618+
619+
TEST_F(msg_oob, ex_oob_ex_oob)
620+
{
621+
sendpair("x", 1, MSG_OOB);
622+
epollpair(true);
623+
siocatmarkpair(true);
624+
625+
recvpair("x", 1, 1, MSG_OOB);
626+
epollpair(false);
627+
siocatmarkpair(true);
628+
629+
sendpair("y", 1, MSG_OOB);
630+
epollpair(true);
631+
siocatmarkpair(true);
632+
633+
recvpair("y", 1, 1, MSG_OOB);
634+
epollpair(false);
635+
siocatmarkpair(true);
636+
637+
tcp_incompliant {
638+
resetpair(false); /* TCP sets -ECONNRESET for ex-OOB. */
639+
}
640+
}
641+
642+
TEST_F(msg_oob, ex_oob_ex_oob_oob)
643+
{
644+
sendpair("x", 1, MSG_OOB);
645+
epollpair(true);
646+
siocatmarkpair(true);
647+
648+
recvpair("x", 1, 1, MSG_OOB);
649+
epollpair(false);
650+
siocatmarkpair(true);
651+
652+
sendpair("y", 1, MSG_OOB);
653+
epollpair(true);
654+
siocatmarkpair(true);
655+
656+
recvpair("y", 1, 1, MSG_OOB);
657+
epollpair(false);
658+
siocatmarkpair(true);
659+
660+
sendpair("z", 1, MSG_OOB);
661+
epollpair(true);
662+
siocatmarkpair(true);
549663
}
550664

551665
TEST_F(msg_oob, ex_oob_ahead_break)
@@ -576,6 +690,10 @@ TEST_F(msg_oob, ex_oob_ahead_break)
576690
recvpair("d", 1, 1, MSG_OOB);
577691
epollpair(false);
578692
siocatmarkpair(true);
693+
694+
tcp_incompliant {
695+
resetpair(false); /* TCP sets -ECONNRESET for ex-OOB. */
696+
}
579697
}
580698

581699
TEST_F(msg_oob, ex_oob_siocatmark)
@@ -595,6 +713,8 @@ TEST_F(msg_oob, ex_oob_siocatmark)
595713
recvpair("hell", 4, 4, 0); /* Intentionally stop at ex-OOB. */
596714
epollpair(true);
597715
siocatmarkpair(false);
716+
717+
resetpair(true);
598718
}
599719

600720
TEST_F(msg_oob, inline_oob)
@@ -612,6 +732,8 @@ TEST_F(msg_oob, inline_oob)
612732
recvpair("x", 1, 1, 0);
613733
epollpair(false);
614734
siocatmarkpair(false);
735+
736+
resetpair(false);
615737
}
616738

617739
TEST_F(msg_oob, inline_oob_break)
@@ -633,6 +755,8 @@ TEST_F(msg_oob, inline_oob_break)
633755
recvpair("o", 1, 1, 0);
634756
epollpair(false);
635757
siocatmarkpair(false);
758+
759+
resetpair(false);
636760
}
637761

638762
TEST_F(msg_oob, inline_oob_ahead_break)
@@ -661,6 +785,8 @@ TEST_F(msg_oob, inline_oob_ahead_break)
661785

662786
epollpair(false);
663787
siocatmarkpair(false);
788+
789+
resetpair(false);
664790
}
665791

666792
TEST_F(msg_oob, inline_ex_oob_break)
@@ -686,6 +812,8 @@ TEST_F(msg_oob, inline_ex_oob_break)
686812
recvpair("rld", 3, 3, 0);
687813
epollpair(false);
688814
siocatmarkpair(false);
815+
816+
resetpair(false);
689817
}
690818

691819
TEST_F(msg_oob, inline_ex_oob_no_drop)
@@ -707,6 +835,8 @@ TEST_F(msg_oob, inline_ex_oob_no_drop)
707835
recvpair("y", 1, 1, 0);
708836
epollpair(false);
709837
siocatmarkpair(false);
838+
839+
resetpair(false);
710840
}
711841

712842
TEST_F(msg_oob, inline_ex_oob_drop)
@@ -731,6 +861,8 @@ TEST_F(msg_oob, inline_ex_oob_drop)
731861
epollpair(false);
732862
siocatmarkpair(false);
733863
}
864+
865+
resetpair(false);
734866
}
735867

736868
TEST_F(msg_oob, inline_ex_oob_siocatmark)
@@ -752,6 +884,8 @@ TEST_F(msg_oob, inline_ex_oob_siocatmark)
752884
recvpair("hell", 4, 4, 0); /* Intentionally stop at ex-OOB. */
753885
epollpair(true);
754886
siocatmarkpair(false);
887+
888+
resetpair(true);
755889
}
756890

757891
TEST_HARNESS_MAIN

0 commit comments

Comments
 (0)