[PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[Vlad Yasevich]

It is possible to enter sctp_cmd_delete_tcb() without having a
primary path.  The situations this most often happens in is
when duplication cookie processing is triggered.  In this
case, we are deleting a temporarily created association that
is not fully populated.   Additially, at the time we
are deleting the offending association, it is really too
late to issue a BUG!

This was introduced by:
commit f9e42b853523cda0732022c2e0473c183f7aec65
	net: sctp: sideeffect: throw BUG if primary_path is NULL

This patch fixes the following observed crash:
[   42.325370] ------------[ cut here ]------------
[   42.329216] kernel BUG at net/sctp/sm_sideeffect.c:863!
[   42.329216] invalid opcode: 0000 [#1] SMP
[   42.329216] Modules linked in: hmac sctp crc32c libcrc32c cls_u32
sch_netem sch_prio rfcomm bnep bluetooth rfkill nfsd auth_rpcgss
oid_registry nfs_acl nfs lockd fscache sunrpc loop joydev hid_generic
usbhid hid snd_intel8x0 snd_ac97_codec snd_pcm snd_page_alloc snd_seq
snd_timer snd_seq_device psmouse snd ohci_pci evdev parport_pc parport
pcspkr serio_raw ohci_hcd ehci_hcd usbcore ac processor thermal_sys
soundcore ac97_bus microcode usb_common button i2c_piix4 i2c_core ext4
crc16 jbd2 mbcache sd_mod sg sr_mod cdrom crc_t10dif crct10dif_common
ata_generic ahci libahci ata_piix e1000 libata scsi_mod
[   42.329216] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.12.0-rc5+ #2
[   42.329216] Hardware name: innotek GmbH VirtualBox, BIOS VirtualBox
12/01/2006
[   42.329216] task: ffffffff81610440 ti: ffffffff81600000 task.ti:
ffffffff81600000
[   42.329216] RIP: 0010:[<ffffffffa03add10>]  [<ffffffffa03add10>]
sctp_do_sm+0x159/0x1091 [sctp]
[   42.329216] RSP: 0018:ffff88007fc03990  EFLAGS: 00010246
[   42.329216] RAX: ffff8800000829c0 RBX: ffff88002fd0a000 RCX:
ffff88002fd0a6e0
[   42.329216] RDX: 0000000000002710 RSI: 0000000000000000 RDI:
ffff88007fc03900
[   42.329216] RBP: ffff88007ca1ce80 R08: ffff88002fd0a6e0 R09:
0000000072a65008
[   42.329216] R10: 0000000072a65008 R11: 519a9b1ce38676a9 R12:
ffff88007fc039e8
[   42.329216] R13: ffff88007fc03a08 R14: 0000000000000000 R15:
ffff88000003dbc0
[   42.329216] FS:  0000000000000000(0000) GS:ffff88007fc00000(0000)
knlGS:0000000000000000
[   42.329216] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   42.329216] CR2: ffffffffff600400 CR3: 000000002fd43000 CR4:
00000000000006f0
[   42.329216] Stack:
[   42.329216]  0000000000000001 0000000000000286 ffff8800615d31c0
0000000100000000
[   42.329216]  0000000a00000001 ffff880075107000 0000000100000003
ffff88000003dbc0
[   42.329216]  0000000000000000 ffff88007d3b7000 ffff8800615d31c0
ffff88007ca1cc80
[   42.329216] Call Trace:
[   42.329216]  <IRQ>
[   42.329216]  [<ffffffffa03b10ac>] ? sctp_assoc_bh_rcv+0xe0/0x11d
[sctp]
[   42.329216]  [<ffffffffa03c1cb2>] ? sctp_rcv+0x7c2/0x896 [sctp]
[   42.329216]  [<ffffffff812eca5b>] ?
ip_local_deliver_finish+0x105/0x17b
[   42.329216]  [<ffffffff812c42d5>] ?
__netif_receive_skb_core+0x44e/0x4c6
[   42.329216]  [<ffffffff812c450f>] ? netif_receive_skb+0x4c/0x7d
[   42.329216]  [<ffffffff812c4c69>] ? napi_gro_receive+0x35/0x76
[   42.329216]  [<ffffffffa007ad4c>] ? e1000_clean_rx_irq+0x330/0x3cd
[e1000]
[   42.329216]  [<ffffffffa0079cc5>] ? e1000_clean+0x5b9/0x725 [e1000]
[   42.329216]  [<ffffffff81051442>] ? autoremove_wake_function+0x9/0x2a
[   42.329216]  [<ffffffff81056e7f>] ? __wake_up_common+0x42/0x78
[   42.329216]  [<ffffffff812c4a15>] ? net_rx_action+0xa2/0x1c6
[   42.329216]  [<ffffffff8103ae04>] ? __do_softirq+0xe8/0x201
[   42.329216]  [<ffffffff813838dc>] ? call_softirq+0x1c/0x30
[   42.329216]  [<ffffffff81003b7c>] ? do_softirq+0x2c/0x60
[   42.329216]  [<ffffffff8103afe2>] ? irq_exit+0x3b/0x7f
[   42.329216]  [<ffffffff81003803>] ? do_IRQ+0x81/0x98
[   42.329216]  [<ffffffff8137d46a>] ? common_interrupt+0x6a/0x6a
[   42.329216]  <EOI>
[   42.329216]  [<ffffffff81008aa3>] ? default_idle+0x15/0x3d
[   42.329216]  [<ffffffff81009021>] ? arch_cpu_idle+0x6/0x17
[   42.329216]  [<ffffffff8106fbad>] ? cpu_startup_entry+0x10d/0x180
[   42.329216]  [<ffffffff816adcd8>] ? start_kernel+0x3be/0x3c9
[   42.329216]  [<ffffffff816ad730>] ? repair_env_string+0x57/0x57
[   42.329216] Code: 50 12 80 fa 0a 75 1a f6 83 dc 07 00 00 02 75 11 8a
80 30 01 00 00 83 e0 03 3c 03 0f 85 1e 0f 00 00 48 83 bb 48 01 00 00 00
75 02 <0f> 0b 48 89 df e8 56 47 01 00 48 89 df e8 e3 41 00 00 e9 fd 0e
[   42.329216] RIP  [<ffffffffa03add10>] sctp_do_sm+0x159/0x1091 [sctp]
[   42.329216]  RSP <ffff88007fc03990>

Reported-by: Mark Thomas <Mark.Thomas@metaswitch.com>
CC: Mark Thomas <Mark.Thomas@metaswitch.com>
CC: Daniel Borkmann <dborkman@redhat.com>
CC: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 net/sctp/sm_sideeffect.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 666c668..1a6eef3 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -860,7 +860,6 @@ static void sctp_cmd_delete_tcb(sctp_cmd_seq_t *cmds,
 	    (!asoc->temp) && (sk->sk_shutdown != SHUTDOWN_MASK))
 		return;
 
-	BUG_ON(asoc->peer.primary_path == NULL);
 	sctp_unhash_established(asoc);
 	sctp_association_free(asoc);
 }
-- 
1.8.3.1

Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[Daniel Borkmann]

On 10/17/2013 07:30 PM, Vlad Yasevich wrote:
> It is possible to enter sctp_cmd_delete_tcb() without having a
> primary path.  The situations this most often happens in is
> when duplication cookie processing is triggered.  In this
> case, we are deleting a temporarily created association that
> is not fully populated.   Additially, at the time we
> are deleting the offending association, it is really too
> late to issue a BUG!
>
> This was introduced by:
> commit f9e42b853523cda0732022c2e0473c183f7aec65
> 	net: sctp: sideeffect: throw BUG if primary_path is NULL

Sure, lets remove it, but then we could still get a WARN() [sure,
better than BUG], if the user at the very same time checks procfs
through sctp_seq_dump_local_addrs(), see discussion we had here [1]:

  It may trigger the crash later if the user performs some action on the
  association that touches the primary. That's the reason why I was
  proposing the checks below.

  With the checks in command interpreter, we are only left with the
  possibility that primary_path changes to NULL during the association
  lifetime, which code audit doesn't support right now.  If that ever
  changes we would at least have a bit more information to go on.

  [1] http://patchwork.ozlabs.org/patch/251099/

> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 666c668..1a6eef3 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -860,7 +860,6 @@ static void sctp_cmd_delete_tcb(sctp_cmd_seq_t *cmds,
>   	    (!asoc->temp) && (sk->sk_shutdown != SHUTDOWN_MASK))
>   		return;
>
> -	BUG_ON(asoc->peer.primary_path == NULL);
>   	sctp_unhash_established(asoc);
>   	sctp_association_free(asoc);
>   }
>

Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[Daniel Borkmann]

On 10/17/2013 08:01 PM, Daniel Borkmann wrote:
> On 10/17/2013 07:30 PM, Vlad Yasevich wrote:
>> It is possible to enter sctp_cmd_delete_tcb() without having a
>> primary path.  The situations this most often happens in is
>> when duplication cookie processing is triggered.  In this
>> case, we are deleting a temporarily created association that
>> is not fully populated.   Additially, at the time we
>> are deleting the offending association, it is really too
>> late to issue a BUG!
>>
>> This was introduced by:
>> commit f9e42b853523cda0732022c2e0473c183f7aec65
>>     net: sctp: sideeffect: throw BUG if primary_path is NULL
>
> Sure, lets remove it, but then we could still get a WARN() [sure,
> better than BUG], if the user at the very same time checks procfs
> through sctp_seq_dump_local_addrs(), see discussion we had here [1]:
>
>   It may trigger the crash later if the user performs some action on the
>   association that touches the primary. That's the reason why I was
>   proposing the checks below.
>
>   With the checks in command interpreter, we are only left with the
>   possibility that primary_path changes to NULL during the association
>   lifetime, which code audit doesn't support right now.  If that ever
>   changes we would at least have a bit more information to go on.
>
>   [1] http://patchwork.ozlabs.org/patch/251099/

Meaning, all I'm saying is that with f9e42b853 we wanted to find exactly
such a case we have right now, that is, that an assoc could enter the
hashtable w/o primary path, no?

Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[Vlad Yasevich]

On 10/17/2013 02:25 PM, Daniel Borkmann wrote:
> On 10/17/2013 08:01 PM, Daniel Borkmann wrote:
>> On 10/17/2013 07:30 PM, Vlad Yasevich wrote:
>>> It is possible to enter sctp_cmd_delete_tcb() without having a
>>> primary path.  The situations this most often happens in is
>>> when duplication cookie processing is triggered.  In this
>>> case, we are deleting a temporarily created association that
>>> is not fully populated.   Additially, at the time we
>>> are deleting the offending association, it is really too
>>> late to issue a BUG!
>>>
>>> This was introduced by:
>>> commit f9e42b853523cda0732022c2e0473c183f7aec65
>>>     net: sctp: sideeffect: throw BUG if primary_path is NULL
>>
>> Sure, lets remove it, but then we could still get a WARN() [sure,
>> better than BUG], if the user at the very same time checks procfs
>> through sctp_seq_dump_local_addrs(), see discussion we had here [1]:
>>
>>   It may trigger the crash later if the user performs some action on the
>>   association that touches the primary. That's the reason why I was
>>   proposing the checks below.
>>
>>   With the checks in command interpreter, we are only left with the
>>   possibility that primary_path changes to NULL during the association
>>   lifetime, which code audit doesn't support right now.  If that ever
>>   changes we would at least have a bit more information to go on.
>>
>>   [1] http://patchwork.ozlabs.org/patch/251099/
>
> Meaning, all I'm saying is that with f9e42b853 we wanted to find exactly
> such a case we have right now, that is, that an assoc could enter the
> hashtable w/o primary path, no?

But it didn't enter a hash table in this case.  SCTP_CMD_NEW_ASOC
was never issued.  The sequence was:
	SCTP_CMD_SET_ASOC
	SCTP_CMD_DELETE_TCB

Such association would never be found through /proc since it was never
hashed.  Such association would never be found the user since it
is only really alive while the packet is processed.  By all rights
it should be marked as 'temp', but it isn't due to cookie processing.

May be we should update cookie processing function to allow it
to create temp associations if so desired.

-vlad

Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[Daniel Borkmann]

On 10/17/2013 08:35 PM, Vlad Yasevich wrote:
> On 10/17/2013 02:25 PM, Daniel Borkmann wrote:
>> On 10/17/2013 08:01 PM, Daniel Borkmann wrote:
>>> On 10/17/2013 07:30 PM, Vlad Yasevich wrote:
>>>> It is possible to enter sctp_cmd_delete_tcb() without having a
>>>> primary path.  The situations this most often happens in is
>>>> when duplication cookie processing is triggered.  In this
>>>> case, we are deleting a temporarily created association that
>>>> is not fully populated.   Additially, at the time we
>>>> are deleting the offending association, it is really too
>>>> late to issue a BUG!
>>>>
>>>> This was introduced by:
>>>> commit f9e42b853523cda0732022c2e0473c183f7aec65
>>>>     net: sctp: sideeffect: throw BUG if primary_path is NULL
>>>
>>> Sure, lets remove it, but then we could still get a WARN() [sure,
>>> better than BUG], if the user at the very same time checks procfs
>>> through sctp_seq_dump_local_addrs(), see discussion we had here [1]:
>>>
>>>   It may trigger the crash later if the user performs some action on the
>>>   association that touches the primary. That's the reason why I was
>>>   proposing the checks below.
>>>
>>>   With the checks in command interpreter, we are only left with the
>>>   possibility that primary_path changes to NULL during the association
>>>   lifetime, which code audit doesn't support right now.  If that ever
>>>   changes we would at least have a bit more information to go on.
>>>
>>>   [1] http://patchwork.ozlabs.org/patch/251099/
>>
>> Meaning, all I'm saying is that with f9e42b853 we wanted to find exactly
>> such a case we have right now, that is, that an assoc could enter the
>> hashtable w/o primary path, no?
>
> But it didn't enter a hash table in this case.  SCTP_CMD_NEW_ASOC
> was never issued.  The sequence was:
>      SCTP_CMD_SET_ASOC
>      SCTP_CMD_DELETE_TCB
>
> Such association would never be found through /proc since it was never
> hashed.  Such association would never be found the user since it
> is only really alive while the packet is processed.  By all rights
> it should be marked as 'temp', but it isn't due to cookie processing.
>
> May be we should update cookie processing function to allow it
> to create temp associations if so desired.

Yes, I think that might be the better way to move on.

> -vlad

Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Thu, 17 Oct 2013 13:30:42 -0400

> It is possible to enter sctp_cmd_delete_tcb() without having a
> primary path.  The situations this most often happens in is
> when duplication cookie processing is triggered.  In this
> case, we are deleting a temporarily created association that
> is not fully populated.   Additially, at the time we
> are deleting the offending association, it is really too
> late to issue a BUG!

Vlad, it looks like you and Daniel are working on an alternative
scheme to handle this issue.  So I just toss this patch for now?

Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

[Vlad Yasevich]

On Oct 18, 2013, at 4:38 PM, David Miller <davem@davemloft.net> wrote:

> From: Vlad Yasevich <vyasevich@gmail.com>
> Date: Thu, 17 Oct 2013 13:30:42 -0400
> 
>> It is possible to enter sctp_cmd_delete_tcb() without having a
>> primary path.  The situations this most often happens in is
>> when duplication cookie processing is triggered.  In this
>> case, we are deleting a temporarily created association that
>> is not fully populated.   Additially, at the time we
>> are deleting the offending association, it is really too
>> late to issue a BUG!
> 
> Vlad, it looks like you and Daniel are working on an alternative
> scheme to handle this issue.  So I just toss this patch for now?

yep.  toss for now.  

thanks
vlad