sctp: fix a BUG_ON on triggered by setting max_autoclose to

Quoted 112 lines · Vlad Yasevich

Quoted 102 lines · Neil Horman

Quoted 101 lines · Vlad Yasevich

Quoted 83 lines · Wang Weidong

since 2692ba61, add the max_autoclose to sysctl. when I setted the max_autoclose to 0. Just do the test_autoclose, it will trigger that:

[  608.056238] ------------[ cut here ]------------
[  608.056244] kernel BUG at /home/wwd/work/linux/net/sctp/sm_sideeffect.c:1488!
[  608.056250] invalid opcode: 0000 [#1] SMP
[  608.056254] Modules linked in: md5 sctp(O) crc32c libcrc32c edd fuse loop dm_mod ipv6 8139too sg 8139cp mii i2c_piix4 i2c_core virtio_balloon intel_agp virtio_pci virtio_ring sr_mod cdrom rtc_cmos joydev hid_generic intel_gtt virtio floppy pcspkr button ext3 jbd mbcache usbhid hid uhci_hcd ehci_hcd usbcore sd_mod usb_common crc_t10dif crct10dif_common processor thermal_sys hwmon scsi_dh_emc scsi_dh_alua scsi_dh_hp_sw scsi_dh_rdac scsi_dh ata_generic ata_piix libata scsi_mod [last unloaded: sctp]
[  608.056310] CPU: 5 PID: 4517 Comm: test_autoclose Tainted: G  R        O 3.13.0-rc1-0.27-default+ #2
[  608.056315] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  608.056319] task: ffff8800372f5590 ti: ffff8800db882000 task.ti: ffff8800db882000
[  608.056323] RIP: 0010:[<ffffffffa033e350>]  [<ffffffffa033e350>] sctp_cmd_interpreter+0x1010/0x1070 [sctp]
[  608.056339] RSP: 0018:ffff880116f43928  EFLAGS: 00010246
[  608.056343] RAX: 0000000000000009 RBX: ffff880116f43ab8 RCX: 0000000000000009
[  608.056349] RDX: 0000000000000003 RSI: 0000000000000000 RDI: ffff880116f43a88
[  608.056353] RBP: ffff880116f439d8 R08: 0000000000002029 R09: 0000000000000001
[  608.056357] R10: 0000000000000000 R11: 0000000000000005 R12: ffff8800db7c9150
[  608.056361] R13: 0000000000000000 R14: ffff8800db7c9000 R15: 0000000000000001
[  608.056365] FS:  00007f942d71c700(0000) GS:ffff880116f40000(0000) knlGS:0000000000000000
[  608.056370] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  608.056373] CR2: 00007f942d324d90 CR3: 00000000db094000 CR4: 00000000000006e0
[  608.056382] Stack:
[  608.056384]  0000000000000018 ffff880116f43988 ffff8800da569600 000000010000000a
[  608.056391]  ffff880037265e40 0000000016f43988 ffff880116f439e8 0000000000000000
[  608.056397]  ffffffffa0365101 0000000000000000 ffff880116f43a28 ffffffff812b7739
[  608.056403] Call Trace:
[  608.056406]  <IRQ>
[  608.056409]
[  608.056421]  [<ffffffff812b7739>] ? __dynamic_pr_debug+0x69/0x80
[  608.056432]  [<ffffffff81097c6d>] ? trace_hardirqs_off+0xd/0x10
[  608.056442]  [<ffffffff814e7258>] ? _raw_spin_unlock_irqrestore+0x58/0x60
[  608.056451]  [<ffffffffa033e3e6>] sctp_side_effects+0x36/0x130 [sctp]
[  608.056459]  [<ffffffffa033e5c7>] sctp_do_sm+0xe7/0x210 [sctp]
[  608.056470]  [<ffffffffa0359860>] ? sctp_rcv+0x780/0x780 [sctp]
[  608.056479]  [<ffffffffa0340a5f>] sctp_endpoint_bh_rcv+0x10f/0x220 [sctp]
[  608.056489]  [<ffffffffa0349991>] sctp_inq_push+0x41/0x60 [sctp]
[  608.056498]  [<ffffffffa03597b5>] sctp_rcv+0x6d5/0x780 [sctp]
[  608.056508]  [<ffffffffa0358e00>] ? sctp_csum_combine+0x10/0x10 [sctp]
[  608.056518]  [<ffffffffa0358df0>] ? sctp_v4_err+0x240/0x240 [sctp]
[  608.056528]  [<ffffffff8145b8a4>] ip_local_deliver_finish+0xf4/0x270
[  608.056533]  [<ffffffff8145b7f0>] ? ip_local_deliver_finish+0x40/0x270
[  608.056538]  [<ffffffff8145b690>] ip_local_deliver+0x80/0x90
[  608.056543]  [<ffffffff8145bbd3>] ip_rcv_finish+0x1b3/0x600
[  608.056547]  [<ffffffff8145ba20>] ? ip_local_deliver_finish+0x270/0x270
[  608.056552]  [<ffffffff8145b04f>] NF_HOOK+0x2f/0x70
[  608.056565]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056570]  [<ffffffff8145b365>] ip_rcv+0x2d5/0x3b0
[  608.056575]  [<ffffffff81425e3f>] __netif_receive_skb_core+0x6ff/0x7a0
[  608.056580]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056585]  [<ffffffff81426108>] ? process_backlog+0x1a8/0x1c0
[  608.056590]  [<ffffffff81425f0b>] __netif_receive_skb+0x2b/0x80
[  608.056595]  [<ffffffff81426018>] process_backlog+0xb8/0x1c0
[  608.056600]  [<ffffffff8142695c>] net_rx_action+0x11c/0x240
[  608.056607]  [<ffffffff81050e18>] __do_softirq+0x118/0x290
[  608.056615]  [<ffffffff814f0e0c>] do_softirq_own_stack+0x1c/0x30
[  608.056618]  <EOI>
[  608.056620]
[  608.056623]  [<ffffffff81050bd5>] do_softirq+0x65/0x70
[  608.056629]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056635]  [<ffffffff810518c3>] local_bh_enable_ip+0xb3/0xc0
[  608.056640]  [<ffffffff814e71af>] _raw_spin_unlock_bh+0x2f/0x40
[  608.056645]  [<ffffffff8140fddc>] release_sock+0x8c/0xa0
[  608.056654]  [<ffffffffa0354c08>] sctp_sendmsg+0x3a8/0xcc0 [sctp]
[  608.056661]  [<ffffffff8129924a>] ? number+0x33a/0x360
[  608.056667]  [<ffffffff81497c3c>] inet_sendmsg+0x9c/0x100
[  608.056672]  [<ffffffff81497ba0>] ? inet_recvmsg+0x110/0x110
[  608.056679]  [<ffffffff8140a387>] sock_sendmsg+0x97/0xc0
[  608.056690]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056695]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056700]  [<ffffffff81417a73>] ? verify_iovec+0x53/0x100
[  608.056705]  [<ffffffff8140abb6>] ___sys_sendmsg+0x416/0x420
[  608.056710]  [<ffffffff81151af6>] ? __do_fault+0x216/0x510
[  608.056715]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056720]  [<ffffffff81095c9e>] ? up_read+0x1e/0x40
[  608.056724]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056729]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056733]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056741]  [<ffffffff8109bb5d>] ? trace_hardirqs_on+0xd/0x10
[  608.056746]  [<ffffffff81051875>] ? local_bh_enable_ip+0x65/0xc0
[  608.056751]  [<ffffffff8140ad94>] __sys_sendmsg+0x44/0x80
[  608.056756]  [<ffffffff8140ade4>] SyS_sendmsg+0x14/0x20
[  608.056761]  [<ffffffff814ef5a2>] system_call_fastpath+0x16/0x1b

-------------------------------------

Quoted 8 lines

The reason is: In test_autoclose set the autoclose to 5 not 0(default). So when we init the association, the (sctp_sock) sp->autoclose is not 0 while asoc's timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] is 0.
So when we process the COOKIE_ACK, the sctp_sf_do_5_1E_ca will be called, that will do that:
	if (asoc->autoclose) //asoc->autoclose is equal sp->autoclose
		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

this looks like a bug in how the timeout is being set. The timeout should be based on the association value, not some sysctl value.

The typical socket option values go like this:
socket value = starts at sysctl, changed by user. assoc value = starts at socket value, may be changed by user.
any timer = starts at assoc value.

Quoted 8 lines

Quoted 2 lines
this seems to be broken for autoclose.
Well, it is, but I think its a reasonable fix for the issue. As I understand the description, the problem is that if autoclose is non-zero, and the default timeout is zero, then you'll get a BUG_ON halt because you need to set a timer that will expire immediately. It seems reasonable to me to make a minimal default for the timeout to be non-zero, since it makes no sense to have a zero auto-close timeout.

Quoted 5 lines

Yes, the default timeout is zero because the timeout is latched by the sysctl value while the association value is not. So, the association value is now out-of-sync with what the system does and we end up lying to the user when they do a getsockopt().

Thats not true, looking at the sctp_association_init code the timeout is defaulted to the minimum of the socket value and the maxium timeout sysctl value, the latter of which is defined as INT_MAX/HZ. That seems reasonable to me in that an association latches both the timeout value and the enabled value from its parent socket at association init time (assoc->autoclose is assigned from sp->autoclose farther down in sctp_association_init).

Quoted 4 lines

If we simply latch the socket value properly, then we would have to worry about this at all. If the socket value is latched at 0, then autoclose is disabled.

We are latching the value properly from what I can see. The problem is that the upper bound on the value we latch can be zero. The only way this bug can occur is if net->sctp.max_autoclose is 0. Note that I'm not talking about the timeout value itself, but its upper bound. We shouldn't be allowed to set the autoclose timeout sysctl range to [0..0], it just doesn't make sense. It should be at least [0..1], and the socket autoclose value can range properly within that. From what I see, thats what this patch does.

Neil

Vlad Yasevich

2013-12-04 14:45 UTC #7 lore raw

Quoted 118 lines · Neil Horman

Quoted 112 lines · Vlad Yasevich

Quoted 102 lines · Neil Horman

Quoted 101 lines · Vlad Yasevich

Quoted 83 lines · Wang Weidong

since 2692ba61, add the max_autoclose to sysctl. when I setted the max_autoclose to 0. Just do the test_autoclose, it will trigger that:

[  608.056238] ------------[ cut here ]------------
[  608.056244] kernel BUG at /home/wwd/work/linux/net/sctp/sm_sideeffect.c:1488!
[  608.056250] invalid opcode: 0000 [#1] SMP
[  608.056254] Modules linked in: md5 sctp(O) crc32c libcrc32c edd fuse loop dm_mod ipv6 8139too sg 8139cp mii i2c_piix4 i2c_core virtio_balloon intel_agp virtio_pci virtio_ring sr_mod cdrom rtc_cmos joydev hid_generic intel_gtt virtio floppy pcspkr button ext3 jbd mbcache usbhid hid uhci_hcd ehci_hcd usbcore sd_mod usb_common crc_t10dif crct10dif_common processor thermal_sys hwmon scsi_dh_emc scsi_dh_alua scsi_dh_hp_sw scsi_dh_rdac scsi_dh ata_generic ata_piix libata scsi_mod [last unloaded: sctp]
[  608.056310] CPU: 5 PID: 4517 Comm: test_autoclose Tainted: G  R        O 3.13.0-rc1-0.27-default+ #2
[  608.056315] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  608.056319] task: ffff8800372f5590 ti: ffff8800db882000 task.ti: ffff8800db882000
[  608.056323] RIP: 0010:[<ffffffffa033e350>]  [<ffffffffa033e350>] sctp_cmd_interpreter+0x1010/0x1070 [sctp]
[  608.056339] RSP: 0018:ffff880116f43928  EFLAGS: 00010246
[  608.056343] RAX: 0000000000000009 RBX: ffff880116f43ab8 RCX: 0000000000000009
[  608.056349] RDX: 0000000000000003 RSI: 0000000000000000 RDI: ffff880116f43a88
[  608.056353] RBP: ffff880116f439d8 R08: 0000000000002029 R09: 0000000000000001
[  608.056357] R10: 0000000000000000 R11: 0000000000000005 R12: ffff8800db7c9150
[  608.056361] R13: 0000000000000000 R14: ffff8800db7c9000 R15: 0000000000000001
[  608.056365] FS:  00007f942d71c700(0000) GS:ffff880116f40000(0000) knlGS:0000000000000000
[  608.056370] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  608.056373] CR2: 00007f942d324d90 CR3: 00000000db094000 CR4: 00000000000006e0
[  608.056382] Stack:
[  608.056384]  0000000000000018 ffff880116f43988 ffff8800da569600 000000010000000a
[  608.056391]  ffff880037265e40 0000000016f43988 ffff880116f439e8 0000000000000000
[  608.056397]  ffffffffa0365101 0000000000000000 ffff880116f43a28 ffffffff812b7739
[  608.056403] Call Trace:
[  608.056406]  <IRQ>
[  608.056409]
[  608.056421]  [<ffffffff812b7739>] ? __dynamic_pr_debug+0x69/0x80
[  608.056432]  [<ffffffff81097c6d>] ? trace_hardirqs_off+0xd/0x10
[  608.056442]  [<ffffffff814e7258>] ? _raw_spin_unlock_irqrestore+0x58/0x60
[  608.056451]  [<ffffffffa033e3e6>] sctp_side_effects+0x36/0x130 [sctp]
[  608.056459]  [<ffffffffa033e5c7>] sctp_do_sm+0xe7/0x210 [sctp]
[  608.056470]  [<ffffffffa0359860>] ? sctp_rcv+0x780/0x780 [sctp]
[  608.056479]  [<ffffffffa0340a5f>] sctp_endpoint_bh_rcv+0x10f/0x220 [sctp]
[  608.056489]  [<ffffffffa0349991>] sctp_inq_push+0x41/0x60 [sctp]
[  608.056498]  [<ffffffffa03597b5>] sctp_rcv+0x6d5/0x780 [sctp]
[  608.056508]  [<ffffffffa0358e00>] ? sctp_csum_combine+0x10/0x10 [sctp]
[  608.056518]  [<ffffffffa0358df0>] ? sctp_v4_err+0x240/0x240 [sctp]
[  608.056528]  [<ffffffff8145b8a4>] ip_local_deliver_finish+0xf4/0x270
[  608.056533]  [<ffffffff8145b7f0>] ? ip_local_deliver_finish+0x40/0x270
[  608.056538]  [<ffffffff8145b690>] ip_local_deliver+0x80/0x90
[  608.056543]  [<ffffffff8145bbd3>] ip_rcv_finish+0x1b3/0x600
[  608.056547]  [<ffffffff8145ba20>] ? ip_local_deliver_finish+0x270/0x270
[  608.056552]  [<ffffffff8145b04f>] NF_HOOK+0x2f/0x70
[  608.056565]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056570]  [<ffffffff8145b365>] ip_rcv+0x2d5/0x3b0
[  608.056575]  [<ffffffff81425e3f>] __netif_receive_skb_core+0x6ff/0x7a0
[  608.056580]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056585]  [<ffffffff81426108>] ? process_backlog+0x1a8/0x1c0
[  608.056590]  [<ffffffff81425f0b>] __netif_receive_skb+0x2b/0x80
[  608.056595]  [<ffffffff81426018>] process_backlog+0xb8/0x1c0
[  608.056600]  [<ffffffff8142695c>] net_rx_action+0x11c/0x240
[  608.056607]  [<ffffffff81050e18>] __do_softirq+0x118/0x290
[  608.056615]  [<ffffffff814f0e0c>] do_softirq_own_stack+0x1c/0x30
[  608.056618]  <EOI>
[  608.056620]
[  608.056623]  [<ffffffff81050bd5>] do_softirq+0x65/0x70
[  608.056629]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056635]  [<ffffffff810518c3>] local_bh_enable_ip+0xb3/0xc0
[  608.056640]  [<ffffffff814e71af>] _raw_spin_unlock_bh+0x2f/0x40
[  608.056645]  [<ffffffff8140fddc>] release_sock+0x8c/0xa0
[  608.056654]  [<ffffffffa0354c08>] sctp_sendmsg+0x3a8/0xcc0 [sctp]
[  608.056661]  [<ffffffff8129924a>] ? number+0x33a/0x360
[  608.056667]  [<ffffffff81497c3c>] inet_sendmsg+0x9c/0x100
[  608.056672]  [<ffffffff81497ba0>] ? inet_recvmsg+0x110/0x110
[  608.056679]  [<ffffffff8140a387>] sock_sendmsg+0x97/0xc0
[  608.056690]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056695]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056700]  [<ffffffff81417a73>] ? verify_iovec+0x53/0x100
[  608.056705]  [<ffffffff8140abb6>] ___sys_sendmsg+0x416/0x420
[  608.056710]  [<ffffffff81151af6>] ? __do_fault+0x216/0x510
[  608.056715]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056720]  [<ffffffff81095c9e>] ? up_read+0x1e/0x40
[  608.056724]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056729]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056733]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056741]  [<ffffffff8109bb5d>] ? trace_hardirqs_on+0xd/0x10
[  608.056746]  [<ffffffff81051875>] ? local_bh_enable_ip+0x65/0xc0
[  608.056751]  [<ffffffff8140ad94>] __sys_sendmsg+0x44/0x80
[  608.056756]  [<ffffffff8140ade4>] SyS_sendmsg+0x14/0x20
[  608.056761]  [<ffffffff814ef5a2>] system_call_fastpath+0x16/0x1b

-------------------------------------

Quoted 8 lines

The reason is: In test_autoclose set the autoclose to 5 not 0(default). So when we init the association, the (sctp_sock) sp->autoclose is not 0 while asoc's timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] is 0.
So when we process the COOKIE_ACK, the sctp_sf_do_5_1E_ca will be called, that will do that:
	if (asoc->autoclose) //asoc->autoclose is equal sp->autoclose
		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

this looks like a bug in how the timeout is being set. The timeout should be based on the association value, not some sysctl value.

The typical socket option values go like this:
socket value = starts at sysctl, changed by user. assoc value = starts at socket value, may be changed by user.
any timer = starts at assoc value.

Quoted 8 lines

Quoted 2 lines
this seems to be broken for autoclose.
Well, it is, but I think its a reasonable fix for the issue. As I understand the description, the problem is that if autoclose is non-zero, and the default timeout is zero, then you'll get a BUG_ON halt because you need to set a timer that will expire immediately. It seems reasonable to me to make a minimal default for the timeout to be non-zero, since it makes no sense to have a zero auto-close timeout.

Quoted 5 lines

Yes, the default timeout is zero because the timeout is latched by the sysctl value while the association value is not. So, the association value is now out-of-sync with what the system does and we end up lying to the user when they do a getsockopt().

Quoted 6 lines

Thats not true, looking at the sctp_association_init code the timeout is defaulted to the minimum of the socket value and the maxium timeout sysctl value, the latter of which is defined as INT_MAX/HZ. That seems reasonable to me in that an association latches both the timeout value and the enabled value from its parent socket at association init time (assoc->autoclose is assigned from sp->autoclose farther down in sctp_association_init).

Right, so let's see what happens. Let the sysctl value be set to 1. The the socket is created and the option is used to set the value to 10. Now, association is created and the timeout is latched to 1. However, the autoclose value of the association is set to 10. So now we are lying to the user about our autoclose value!

This is what's causing the problem. The timeout value is out of sync what what the requested timeout value is.

Now, if we latch the socket value correctly, then everything just starts to work, even for sysctl value of 0, and we stop lying to the user on getsockopt() calls. This is the real bug that causing the BUG_ON.

Quoted 5 lines

Quoted 5 lines
If we simply latch the socket value properly, then we would have to worry about this at all. If the socket value is latched at 0, then autoclose is disabled.

Quoted 7 lines

We are latching the value properly from what I can see. The problem is that the upper bound on the value we latch can be zero. The only way this bug can occur is if net->sctp.max_autoclose is 0. Note that I'm not talking about the timeout value itself, but its upper bound. We shouldn't be allowed to set the autoclose timeout sysctl range to [0..0], it just doesn't make sense. It should be at least [0..1], and the socket autoclose value can range properly within that. From what I see, thats what this patch does.

We don't allow the sysctl range of [0..0]. We allow the range of [0..max_autoclose_max]. This patch tries to change the range to [1..max_autoclose_max]. This may be a correct thing to do, but it is another issue. That fact this change fixes the BUG_ON is a side-effect. The real issue is in how we set set the timer value.

-vlad

Quoted 3 lines

Neil

Neil Horman

2013-12-04 18:56 UTC #8 lore raw

Quoted 126 lines · Vlad Yasevich

Quoted 118 lines · Neil Horman

Quoted 112 lines · Vlad Yasevich

Quoted 102 lines · Neil Horman

Quoted 101 lines · Vlad Yasevich

Quoted 83 lines · Wang Weidong

since 2692ba61, add the max_autoclose to sysctl. when I setted the max_autoclose to 0. Just do the test_autoclose, it will trigger that:

[  608.056238] ------------[ cut here ]------------
[  608.056244] kernel BUG at /home/wwd/work/linux/net/sctp/sm_sideeffect.c:1488!
[  608.056250] invalid opcode: 0000 [#1] SMP
[  608.056254] Modules linked in: md5 sctp(O) crc32c libcrc32c edd fuse loop dm_mod ipv6 8139too sg 8139cp mii i2c_piix4 i2c_core virtio_balloon intel_agp virtio_pci virtio_ring sr_mod cdrom rtc_cmos joydev hid_generic intel_gtt virtio floppy pcspkr button ext3 jbd mbcache usbhid hid uhci_hcd ehci_hcd usbcore sd_mod usb_common crc_t10dif crct10dif_common processor thermal_sys hwmon scsi_dh_emc scsi_dh_alua scsi_dh_hp_sw scsi_dh_rdac scsi_dh ata_generic ata_piix libata scsi_mod [last unloaded: sctp]
[  608.056310] CPU: 5 PID: 4517 Comm: test_autoclose Tainted: G  R        O 3.13.0-rc1-0.27-default+ #2
[  608.056315] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  608.056319] task: ffff8800372f5590 ti: ffff8800db882000 task.ti: ffff8800db882000
[  608.056323] RIP: 0010:[<ffffffffa033e350>]  [<ffffffffa033e350>] sctp_cmd_interpreter+0x1010/0x1070 [sctp]
[  608.056339] RSP: 0018:ffff880116f43928  EFLAGS: 00010246
[  608.056343] RAX: 0000000000000009 RBX: ffff880116f43ab8 RCX: 0000000000000009
[  608.056349] RDX: 0000000000000003 RSI: 0000000000000000 RDI: ffff880116f43a88
[  608.056353] RBP: ffff880116f439d8 R08: 0000000000002029 R09: 0000000000000001
[  608.056357] R10: 0000000000000000 R11: 0000000000000005 R12: ffff8800db7c9150
[  608.056361] R13: 0000000000000000 R14: ffff8800db7c9000 R15: 0000000000000001
[  608.056365] FS:  00007f942d71c700(0000) GS:ffff880116f40000(0000) knlGS:0000000000000000
[  608.056370] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  608.056373] CR2: 00007f942d324d90 CR3: 00000000db094000 CR4: 00000000000006e0
[  608.056382] Stack:
[  608.056384]  0000000000000018 ffff880116f43988 ffff8800da569600 000000010000000a
[  608.056391]  ffff880037265e40 0000000016f43988 ffff880116f439e8 0000000000000000
[  608.056397]  ffffffffa0365101 0000000000000000 ffff880116f43a28 ffffffff812b7739
[  608.056403] Call Trace:
[  608.056406]  <IRQ>
[  608.056409]
[  608.056421]  [<ffffffff812b7739>] ? __dynamic_pr_debug+0x69/0x80
[  608.056432]  [<ffffffff81097c6d>] ? trace_hardirqs_off+0xd/0x10
[  608.056442]  [<ffffffff814e7258>] ? _raw_spin_unlock_irqrestore+0x58/0x60
[  608.056451]  [<ffffffffa033e3e6>] sctp_side_effects+0x36/0x130 [sctp]
[  608.056459]  [<ffffffffa033e5c7>] sctp_do_sm+0xe7/0x210 [sctp]
[  608.056470]  [<ffffffffa0359860>] ? sctp_rcv+0x780/0x780 [sctp]
[  608.056479]  [<ffffffffa0340a5f>] sctp_endpoint_bh_rcv+0x10f/0x220 [sctp]
[  608.056489]  [<ffffffffa0349991>] sctp_inq_push+0x41/0x60 [sctp]
[  608.056498]  [<ffffffffa03597b5>] sctp_rcv+0x6d5/0x780 [sctp]
[  608.056508]  [<ffffffffa0358e00>] ? sctp_csum_combine+0x10/0x10 [sctp]
[  608.056518]  [<ffffffffa0358df0>] ? sctp_v4_err+0x240/0x240 [sctp]
[  608.056528]  [<ffffffff8145b8a4>] ip_local_deliver_finish+0xf4/0x270
[  608.056533]  [<ffffffff8145b7f0>] ? ip_local_deliver_finish+0x40/0x270
[  608.056538]  [<ffffffff8145b690>] ip_local_deliver+0x80/0x90
[  608.056543]  [<ffffffff8145bbd3>] ip_rcv_finish+0x1b3/0x600
[  608.056547]  [<ffffffff8145ba20>] ? ip_local_deliver_finish+0x270/0x270
[  608.056552]  [<ffffffff8145b04f>] NF_HOOK+0x2f/0x70
[  608.056565]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056570]  [<ffffffff8145b365>] ip_rcv+0x2d5/0x3b0
[  608.056575]  [<ffffffff81425e3f>] __netif_receive_skb_core+0x6ff/0x7a0
[  608.056580]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056585]  [<ffffffff81426108>] ? process_backlog+0x1a8/0x1c0
[  608.056590]  [<ffffffff81425f0b>] __netif_receive_skb+0x2b/0x80
[  608.056595]  [<ffffffff81426018>] process_backlog+0xb8/0x1c0
[  608.056600]  [<ffffffff8142695c>] net_rx_action+0x11c/0x240
[  608.056607]  [<ffffffff81050e18>] __do_softirq+0x118/0x290
[  608.056615]  [<ffffffff814f0e0c>] do_softirq_own_stack+0x1c/0x30
[  608.056618]  <EOI>
[  608.056620]
[  608.056623]  [<ffffffff81050bd5>] do_softirq+0x65/0x70
[  608.056629]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056635]  [<ffffffff810518c3>] local_bh_enable_ip+0xb3/0xc0
[  608.056640]  [<ffffffff814e71af>] _raw_spin_unlock_bh+0x2f/0x40
[  608.056645]  [<ffffffff8140fddc>] release_sock+0x8c/0xa0
[  608.056654]  [<ffffffffa0354c08>] sctp_sendmsg+0x3a8/0xcc0 [sctp]
[  608.056661]  [<ffffffff8129924a>] ? number+0x33a/0x360
[  608.056667]  [<ffffffff81497c3c>] inet_sendmsg+0x9c/0x100
[  608.056672]  [<ffffffff81497ba0>] ? inet_recvmsg+0x110/0x110
[  608.056679]  [<ffffffff8140a387>] sock_sendmsg+0x97/0xc0
[  608.056690]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056695]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056700]  [<ffffffff81417a73>] ? verify_iovec+0x53/0x100
[  608.056705]  [<ffffffff8140abb6>] ___sys_sendmsg+0x416/0x420
[  608.056710]  [<ffffffff81151af6>] ? __do_fault+0x216/0x510
[  608.056715]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056720]  [<ffffffff81095c9e>] ? up_read+0x1e/0x40
[  608.056724]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056729]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056733]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056741]  [<ffffffff8109bb5d>] ? trace_hardirqs_on+0xd/0x10
[  608.056746]  [<ffffffff81051875>] ? local_bh_enable_ip+0x65/0xc0
[  608.056751]  [<ffffffff8140ad94>] __sys_sendmsg+0x44/0x80
[  608.056756]  [<ffffffff8140ade4>] SyS_sendmsg+0x14/0x20
[  608.056761]  [<ffffffff814ef5a2>] system_call_fastpath+0x16/0x1b

-------------------------------------

Quoted 8 lines

The reason is: In test_autoclose set the autoclose to 5 not 0(default). So when we init the association, the (sctp_sock) sp->autoclose is not 0 while asoc's timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] is 0.
So when we process the COOKIE_ACK, the sctp_sf_do_5_1E_ca will be called, that will do that:
	if (asoc->autoclose) //asoc->autoclose is equal sp->autoclose
		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

this looks like a bug in how the timeout is being set. The timeout should be based on the association value, not some sysctl value.

The typical socket option values go like this:
socket value = starts at sysctl, changed by user. assoc value = starts at socket value, may be changed by user.
any timer = starts at assoc value.

Quoted 8 lines

Quoted 2 lines
this seems to be broken for autoclose.
Well, it is, but I think its a reasonable fix for the issue. As I understand the description, the problem is that if autoclose is non-zero, and the default timeout is zero, then you'll get a BUG_ON halt because you need to set a timer that will expire immediately. It seems reasonable to me to make a minimal default for the timeout to be non-zero, since it makes no sense to have a zero auto-close timeout.

Quoted 5 lines

Yes, the default timeout is zero because the timeout is latched by the sysctl value while the association value is not. So, the association value is now out-of-sync with what the system does and we end up lying to the user when they do a getsockopt().

Quoted 6 lines

Thats not true, looking at the sctp_association_init code the timeout is defaulted to the minimum of the socket value and the maxium timeout sysctl value, the latter of which is defined as INT_MAX/HZ. That seems reasonable to me in that an association latches both the timeout value and the enabled value from its parent socket at association init time (assoc->autoclose is assigned from sp->autoclose farther down in sctp_association_init).

Quoted 6 lines

Right, so let's see what happens. Let the sysctl value be set to 1. The the socket is created and the option is used to set the value to 10. Now, association is created and the timeout is latched to 1. However, the autoclose value of the association is set to 10. So now we are lying to the user about our autoclose value!

I think thats not really a problem here. The sysctl in question is designed to do exactly what you describe, and in your scenario above the user has change the sysctl from the default setting of MAX_INT/HZ to 1, so they know that the autocolse timeout is going to be limited. I agree that the fact that getsockopt is going to return the value 10 instead of 1 is an irritant, but its never guaranteed to match what an associations actual timeout is, because, as you note, its latched at the time the association is created. Its no different than setting SOCK_AUTOCLOSE to 10, creating an association, then changing SOCK_AUTOCLOSE to 20. The getsockopt operation will return 20 even though the existing association isn't using that as a timeout value.

Quoted 12 lines

This is what's causing the problem. The timeout value is out of sync what what the requested timeout value is.
Now, if we latch the socket value correctly, then everything just starts to work, even for sysctl value of 0, and we stop lying to the user on getsockopt() calls. This is the real bug that causing the BUG_ON.
Quoted 5 lines
Quoted 5 lines
If we simply latch the socket value properly, then we would have to worry about this at all. If the socket value is latched at 0, then autoclose is disabled.

Ok, so whats "correct" here? Are you proposing that we remove the max_autoclose sysctl entirely, or shall we check the value passed into the autoclose setsockopt against the sysctl value, to ensure that users don't set a value larger than what the max specifies, lest we continue to 'lie' to them by letting them set a autoclose timeout larger than what we intend to set when we initalize an association? Regardless of what we do here, Theres still no guarantee that the socket level value will match any given association, and thats just the way it goes.

What we should do, regardless, is remove assoc->autoclose entirely, and replace all references to it with references to asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]
That way we use a single canonical place to test the autoclose value (since assoc->autoclose is only used as a boolean test anyway). Writing it down and reading it back to myself, that may actually be congruent with what you mean when you say that we should latch the socket value properly.

Neil

Quoted 25 lines

Quoted 7 lines
We are latching the value properly from what I can see. The problem is that the upper bound on the value we latch can be zero. The only way this bug can occur is if net->sctp.max_autoclose is 0. Note that I'm not talking about the timeout value itself, but its upper bound. We shouldn't be allowed to set the autoclose timeout sysctl range to [0..0], it just doesn't make sense. It should be at least [0..1], and the socket autoclose value can range properly within that. From what I see, thats what this patch does.
We don't allow the sysctl range of [0..0]. We allow the range of [0..max_autoclose_max]. This patch tries to change the range to [1..max_autoclose_max]. This may be a correct thing to do, but it is another issue. That fact this change fixes the BUG_ON is a side-effect. The real issue is in how we set set the timer value.
-vlad
Quoted 3 lines
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vlad Yasevich

2013-12-05 16:00 UTC #9 lore raw

Quoted 133 lines · Neil Horman

Quoted 126 lines · Vlad Yasevich
Quoted 118 lines · Neil Horman
Quoted 112 lines · Vlad Yasevich
Quoted 102 lines · Neil Horman
Quoted 101 lines · Vlad Yasevich
Quoted 83 lines · Wang Weidong
since 2692ba61, add the max_autoclose to sysctl. when I setted the max_autoclose to 0. Just do the test_autoclose, it will trigger that:
[  608.056238] ------------[ cut here ]------------
[  608.056244] kernel BUG at /home/wwd/work/linux/net/sctp/sm_sideeffect.c:1488!
[  608.056250] invalid opcode: 0000 [#1] SMP
[  608.056254] Modules linked in: md5 sctp(O) crc32c libcrc32c edd fuse loop dm_mod ipv6 8139too sg 8139cp mii i2c_piix4 i2c_core virtio_balloon intel_agp virtio_pci virtio_ring sr_mod cdrom rtc_cmos joydev hid_generic intel_gtt virtio floppy pcspkr button ext3 jbd mbcache usbhid hid uhci_hcd ehci_hcd usbcore sd_mod usb_common crc_t10dif crct10dif_common processor thermal_sys hwmon scsi_dh_emc scsi_dh_alua scsi_dh_hp_sw scsi_dh_rdac scsi_dh ata_generic ata_piix libata scsi_mod [last unloaded: sctp]
[  608.056310] CPU: 5 PID: 4517 Comm: test_autoclose Tainted: G  R        O 3.13.0-rc1-0.27-default+ #2
[  608.056315] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  608.056319] task: ffff8800372f5590 ti: ffff8800db882000 task.ti: ffff8800db882000
[  608.056323] RIP: 0010:[<ffffffffa033e350>]  [<ffffffffa033e350>] sctp_cmd_interpreter+0x1010/0x1070 [sctp]
[  608.056339] RSP: 0018:ffff880116f43928  EFLAGS: 00010246
[  608.056343] RAX: 0000000000000009 RBX: ffff880116f43ab8 RCX: 0000000000000009
[  608.056349] RDX: 0000000000000003 RSI: 0000000000000000 RDI: ffff880116f43a88
[  608.056353] RBP: ffff880116f439d8 R08: 0000000000002029 R09: 0000000000000001
[  608.056357] R10: 0000000000000000 R11: 0000000000000005 R12: ffff8800db7c9150
[  608.056361] R13: 0000000000000000 R14: ffff8800db7c9000 R15: 0000000000000001
[  608.056365] FS:  00007f942d71c700(0000) GS:ffff880116f40000(0000) knlGS:0000000000000000
[  608.056370] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  608.056373] CR2: 00007f942d324d90 CR3: 00000000db094000 CR4: 00000000000006e0
[  608.056382] Stack:
[  608.056384]  0000000000000018 ffff880116f43988 ffff8800da569600 000000010000000a
[  608.056391]  ffff880037265e40 0000000016f43988 ffff880116f439e8 0000000000000000
[  608.056397]  ffffffffa0365101 0000000000000000 ffff880116f43a28 ffffffff812b7739
[  608.056403] Call Trace:
[  608.056406]  <IRQ>
[  608.056409]
[  608.056421]  [<ffffffff812b7739>] ? __dynamic_pr_debug+0x69/0x80
[  608.056432]  [<ffffffff81097c6d>] ? trace_hardirqs_off+0xd/0x10
[  608.056442]  [<ffffffff814e7258>] ? _raw_spin_unlock_irqrestore+0x58/0x60
[  608.056451]  [<ffffffffa033e3e6>] sctp_side_effects+0x36/0x130 [sctp]
[  608.056459]  [<ffffffffa033e5c7>] sctp_do_sm+0xe7/0x210 [sctp]
[  608.056470]  [<ffffffffa0359860>] ? sctp_rcv+0x780/0x780 [sctp]
[  608.056479]  [<ffffffffa0340a5f>] sctp_endpoint_bh_rcv+0x10f/0x220 [sctp]
[  608.056489]  [<ffffffffa0349991>] sctp_inq_push+0x41/0x60 [sctp]
[  608.056498]  [<ffffffffa03597b5>] sctp_rcv+0x6d5/0x780 [sctp]
[  608.056508]  [<ffffffffa0358e00>] ? sctp_csum_combine+0x10/0x10 [sctp]
[  608.056518]  [<ffffffffa0358df0>] ? sctp_v4_err+0x240/0x240 [sctp]
[  608.056528]  [<ffffffff8145b8a4>] ip_local_deliver_finish+0xf4/0x270
[  608.056533]  [<ffffffff8145b7f0>] ? ip_local_deliver_finish+0x40/0x270
[  608.056538]  [<ffffffff8145b690>] ip_local_deliver+0x80/0x90
[  608.056543]  [<ffffffff8145bbd3>] ip_rcv_finish+0x1b3/0x600
[  608.056547]  [<ffffffff8145ba20>] ? ip_local_deliver_finish+0x270/0x270
[  608.056552]  [<ffffffff8145b04f>] NF_HOOK+0x2f/0x70
[  608.056565]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056570]  [<ffffffff8145b365>] ip_rcv+0x2d5/0x3b0
[  608.056575]  [<ffffffff81425e3f>] __netif_receive_skb_core+0x6ff/0x7a0
[  608.056580]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056585]  [<ffffffff81426108>] ? process_backlog+0x1a8/0x1c0
[  608.056590]  [<ffffffff81425f0b>] __netif_receive_skb+0x2b/0x80
[  608.056595]  [<ffffffff81426018>] process_backlog+0xb8/0x1c0
[  608.056600]  [<ffffffff8142695c>] net_rx_action+0x11c/0x240
[  608.056607]  [<ffffffff81050e18>] __do_softirq+0x118/0x290
[  608.056615]  [<ffffffff814f0e0c>] do_softirq_own_stack+0x1c/0x30
[  608.056618]  <EOI>
[  608.056620]
[  608.056623]  [<ffffffff81050bd5>] do_softirq+0x65/0x70
[  608.056629]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056635]  [<ffffffff810518c3>] local_bh_enable_ip+0xb3/0xc0
[  608.056640]  [<ffffffff814e71af>] _raw_spin_unlock_bh+0x2f/0x40
[  608.056645]  [<ffffffff8140fddc>] release_sock+0x8c/0xa0
[  608.056654]  [<ffffffffa0354c08>] sctp_sendmsg+0x3a8/0xcc0 [sctp]
[  608.056661]  [<ffffffff8129924a>] ? number+0x33a/0x360
[  608.056667]  [<ffffffff81497c3c>] inet_sendmsg+0x9c/0x100
[  608.056672]  [<ffffffff81497ba0>] ? inet_recvmsg+0x110/0x110
[  608.056679]  [<ffffffff8140a387>] sock_sendmsg+0x97/0xc0
[  608.056690]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056695]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056700]  [<ffffffff81417a73>] ? verify_iovec+0x53/0x100
[  608.056705]  [<ffffffff8140abb6>] ___sys_sendmsg+0x416/0x420
[  608.056710]  [<ffffffff81151af6>] ? __do_fault+0x216/0x510
[  608.056715]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056720]  [<ffffffff81095c9e>] ? up_read+0x1e/0x40
[  608.056724]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056729]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056733]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056741]  [<ffffffff8109bb5d>] ? trace_hardirqs_on+0xd/0x10
[  608.056746]  [<ffffffff81051875>] ? local_bh_enable_ip+0x65/0xc0
[  608.056751]  [<ffffffff8140ad94>] __sys_sendmsg+0x44/0x80
[  608.056756]  [<ffffffff8140ade4>] SyS_sendmsg+0x14/0x20
[  608.056761]  [<ffffffff814ef5a2>] system_call_fastpath+0x16/0x1b
-------------------------------------
Quoted 8 lines
The reason is: In test_autoclose set the autoclose to 5 not 0(default). So when we init the association, the (sctp_sock) sp->autoclose is not 0 while asoc's timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] is 0.
So when we process the COOKIE_ACK, the sctp_sf_do_5_1E_ca will be called, that will do that:
	if (asoc->autoclose) //asoc->autoclose is equal sp->autoclose
		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
this looks like a bug in how the timeout is being set. The timeout should be based on the association value, not some sysctl value.
The typical socket option values go like this:
socket value = starts at sysctl, changed by user. assoc value = starts at socket value, may be changed by user.
any timer = starts at assoc value.
Quoted 8 lines
Quoted 2 lines
this seems to be broken for autoclose.
Well, it is, but I think its a reasonable fix for the issue. As I understand the description, the problem is that if autoclose is non-zero, and the default timeout is zero, then you'll get a BUG_ON halt because you need to set a timer that will expire immediately. It seems reasonable to me to make a minimal default for the timeout to be non-zero, since it makes no sense to have a zero auto-close timeout.
Quoted 5 lines
Yes, the default timeout is zero because the timeout is latched by the sysctl value while the association value is not. So, the association value is now out-of-sync with what the system does and we end up lying to the user when they do a getsockopt().
Quoted 6 lines
Thats not true, looking at the sctp_association_init code the timeout is defaulted to the minimum of the socket value and the maxium timeout sysctl value, the latter of which is defined as INT_MAX/HZ. That seems reasonable to me in that an association latches both the timeout value and the enabled value from its parent socket at association init time (assoc->autoclose is assigned from sp->autoclose farther down in sctp_association_init).
Quoted 6 lines
Right, so let's see what happens. Let the sysctl value be set to 1. The the socket is created and the option is used to set the value to 10. Now, association is created and the timeout is latched to 1. However, the autoclose value of the association is set to 10. So now we are lying to the user about our autoclose value!

Quoted 10 lines

I think thats not really a problem here. The sysctl in question is designed to do exactly what you describe, and in your scenario above the user has change the sysctl from the default setting of MAX_INT/HZ to 1, so they know that the autocolse timeout is going to be limited. I agree that the fact that getsockopt is going to return the value 10 instead of 1 is an irritant, but its never guaranteed to match what an associations actual timeout is, because, as you note, its latched at the time the association is created. Its no different than setting SOCK_AUTOCLOSE to 10, creating an association, then changing SOCK_AUTOCLOSE to 20. The getsockopt operation will return 20 even though the existing association isn't using that as a timeout value.

Yes, the above is absolutely true for multiple calls to set autoclose. However, for a single call to autoclose, we should return the value currently in use, not the value that use the set and we may be ignoring completely.

Quoted 13 lines

Quoted 13 lines
This is what's causing the problem. The timeout value is out of sync what what the requested timeout value is.
Now, if we latch the socket value correctly, then everything just starts to work, even for sysctl value of 0, and we stop lying to the user on getsockopt() calls. This is the real bug that causing the BUG_ON.
Quoted 5 lines
Quoted 5 lines
If we simply latch the socket value properly, then we would have to worry about this at all. If the socket value is latched at 0, then autoclose is disabled.

Quoted 6 lines

Ok, so whats "correct" here? Are you proposing that we remove the max_autoclose sysctl entirely, or shall we check the value passed into the autoclose setsockopt against the sysctl value, to ensure that users don't set a value larger than what the max specifies, lest we continue to 'lie' to them by letting them set a autoclose timeout larger than what we intend to set when we initalize an association?

Yes, that's what we should do. Essentially treat this similar to how the stack SO_{SND|RCV}BUF. Might even log a warning to say autoclose is limited by sysctl.

Quoted 4 lines

Regardless of what we do here, Theres still no guarantee that
the socket level value will match any given association, and thats just the way it goes.

That's ok, as long as it uses a valid value and the association timer is set based on the value in the socket.

Quoted 8 lines

What we should do, regardless, is remove assoc->autoclose entirely, and replace all references to it with references to asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]
That way we use a single canonical place to test the autoclose value (since assoc->autoclose is only used as a boolean test anyway). Writing it down and reading it back to myself, that may actually be congruent with what you mean when you say that we should latch the socket value properly.

That's fine to. So we would have a socket value that is properly limited by sysctl and returns to the user the value new association will use. We would also have the timer value in the association that is initialized with the socket value. The timer value of 0 means autoclose is disabled. Does that make sense to you?

-vlad

Quoted 27 lines

Neil
Quoted 25 lines
Quoted 7 lines
We are latching the value properly from what I can see. The problem is that the upper bound on the value we latch can be zero. The only way this bug can occur is if net->sctp.max_autoclose is 0. Note that I'm not talking about the timeout value itself, but its upper bound. We shouldn't be allowed to set the autoclose timeout sysctl range to [0..0], it just doesn't make sense. It should be at least [0..1], and the socket autoclose value can range properly within that. From what I see, thats what this patch does.
We don't allow the sysctl range of [0..0]. We allow the range of [0..max_autoclose_max]. This patch tries to change the range to [1..max_autoclose_max]. This may be a correct thing to do, but it is another issue. That fact this change fixes the BUG_ON is a side-effect. The real issue is in how we set set the timer value.
-vlad
Quoted 3 lines
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2013-12-05 20:43 UTC #10 lore raw

Quoted 191 lines · Vlad Yasevich

Quoted 133 lines · Neil Horman
Quoted 126 lines · Vlad Yasevich
Quoted 118 lines · Neil Horman
Quoted 112 lines · Vlad Yasevich
Quoted 102 lines · Neil Horman
Quoted 101 lines · Vlad Yasevich
Quoted 83 lines · Wang Weidong
since 2692ba61, add the max_autoclose to sysctl. when I setted the max_autoclose to 0. Just do the test_autoclose, it will trigger that:
[  608.056238] ------------[ cut here ]------------
[  608.056244] kernel BUG at /home/wwd/work/linux/net/sctp/sm_sideeffect.c:1488!
[  608.056250] invalid opcode: 0000 [#1] SMP
[  608.056254] Modules linked in: md5 sctp(O) crc32c libcrc32c edd fuse loop dm_mod ipv6 8139too sg 8139cp mii i2c_piix4 i2c_core virtio_balloon intel_agp virtio_pci virtio_ring sr_mod cdrom rtc_cmos joydev hid_generic intel_gtt virtio floppy pcspkr button ext3 jbd mbcache usbhid hid uhci_hcd ehci_hcd usbcore sd_mod usb_common crc_t10dif crct10dif_common processor thermal_sys hwmon scsi_dh_emc scsi_dh_alua scsi_dh_hp_sw scsi_dh_rdac scsi_dh ata_generic ata_piix libata scsi_mod [last unloaded: sctp]
[  608.056310] CPU: 5 PID: 4517 Comm: test_autoclose Tainted: G  R        O 3.13.0-rc1-0.27-default+ #2
[  608.056315] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  608.056319] task: ffff8800372f5590 ti: ffff8800db882000 task.ti: ffff8800db882000
[  608.056323] RIP: 0010:[<ffffffffa033e350>]  [<ffffffffa033e350>] sctp_cmd_interpreter+0x1010/0x1070 [sctp]
[  608.056339] RSP: 0018:ffff880116f43928  EFLAGS: 00010246
[  608.056343] RAX: 0000000000000009 RBX: ffff880116f43ab8 RCX: 0000000000000009
[  608.056349] RDX: 0000000000000003 RSI: 0000000000000000 RDI: ffff880116f43a88
[  608.056353] RBP: ffff880116f439d8 R08: 0000000000002029 R09: 0000000000000001
[  608.056357] R10: 0000000000000000 R11: 0000000000000005 R12: ffff8800db7c9150
[  608.056361] R13: 0000000000000000 R14: ffff8800db7c9000 R15: 0000000000000001
[  608.056365] FS:  00007f942d71c700(0000) GS:ffff880116f40000(0000) knlGS:0000000000000000
[  608.056370] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  608.056373] CR2: 00007f942d324d90 CR3: 00000000db094000 CR4: 00000000000006e0
[  608.056382] Stack:
[  608.056384]  0000000000000018 ffff880116f43988 ffff8800da569600 000000010000000a
[  608.056391]  ffff880037265e40 0000000016f43988 ffff880116f439e8 0000000000000000
[  608.056397]  ffffffffa0365101 0000000000000000 ffff880116f43a28 ffffffff812b7739
[  608.056403] Call Trace:
[  608.056406]  <IRQ>
[  608.056409]
[  608.056421]  [<ffffffff812b7739>] ? __dynamic_pr_debug+0x69/0x80
[  608.056432]  [<ffffffff81097c6d>] ? trace_hardirqs_off+0xd/0x10
[  608.056442]  [<ffffffff814e7258>] ? _raw_spin_unlock_irqrestore+0x58/0x60
[  608.056451]  [<ffffffffa033e3e6>] sctp_side_effects+0x36/0x130 [sctp]
[  608.056459]  [<ffffffffa033e5c7>] sctp_do_sm+0xe7/0x210 [sctp]
[  608.056470]  [<ffffffffa0359860>] ? sctp_rcv+0x780/0x780 [sctp]
[  608.056479]  [<ffffffffa0340a5f>] sctp_endpoint_bh_rcv+0x10f/0x220 [sctp]
[  608.056489]  [<ffffffffa0349991>] sctp_inq_push+0x41/0x60 [sctp]
[  608.056498]  [<ffffffffa03597b5>] sctp_rcv+0x6d5/0x780 [sctp]
[  608.056508]  [<ffffffffa0358e00>] ? sctp_csum_combine+0x10/0x10 [sctp]
[  608.056518]  [<ffffffffa0358df0>] ? sctp_v4_err+0x240/0x240 [sctp]
[  608.056528]  [<ffffffff8145b8a4>] ip_local_deliver_finish+0xf4/0x270
[  608.056533]  [<ffffffff8145b7f0>] ? ip_local_deliver_finish+0x40/0x270
[  608.056538]  [<ffffffff8145b690>] ip_local_deliver+0x80/0x90
[  608.056543]  [<ffffffff8145bbd3>] ip_rcv_finish+0x1b3/0x600
[  608.056547]  [<ffffffff8145ba20>] ? ip_local_deliver_finish+0x270/0x270
[  608.056552]  [<ffffffff8145b04f>] NF_HOOK+0x2f/0x70
[  608.056565]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056570]  [<ffffffff8145b365>] ip_rcv+0x2d5/0x3b0
[  608.056575]  [<ffffffff81425e3f>] __netif_receive_skb_core+0x6ff/0x7a0
[  608.056580]  [<ffffffff81425853>] ? __netif_receive_skb_core+0x113/0x7a0
[  608.056585]  [<ffffffff81426108>] ? process_backlog+0x1a8/0x1c0
[  608.056590]  [<ffffffff81425f0b>] __netif_receive_skb+0x2b/0x80
[  608.056595]  [<ffffffff81426018>] process_backlog+0xb8/0x1c0
[  608.056600]  [<ffffffff8142695c>] net_rx_action+0x11c/0x240
[  608.056607]  [<ffffffff81050e18>] __do_softirq+0x118/0x290
[  608.056615]  [<ffffffff814f0e0c>] do_softirq_own_stack+0x1c/0x30
[  608.056618]  <EOI>
[  608.056620]
[  608.056623]  [<ffffffff81050bd5>] do_softirq+0x65/0x70
[  608.056629]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056635]  [<ffffffff810518c3>] local_bh_enable_ip+0xb3/0xc0
[  608.056640]  [<ffffffff814e71af>] _raw_spin_unlock_bh+0x2f/0x40
[  608.056645]  [<ffffffff8140fddc>] release_sock+0x8c/0xa0
[  608.056654]  [<ffffffffa0354c08>] sctp_sendmsg+0x3a8/0xcc0 [sctp]
[  608.056661]  [<ffffffff8129924a>] ? number+0x33a/0x360
[  608.056667]  [<ffffffff81497c3c>] inet_sendmsg+0x9c/0x100
[  608.056672]  [<ffffffff81497ba0>] ? inet_recvmsg+0x110/0x110
[  608.056679]  [<ffffffff8140a387>] sock_sendmsg+0x97/0xc0
[  608.056690]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056695]  [<ffffffff8114caae>] ? might_fault+0x3e/0x90
[  608.056700]  [<ffffffff81417a73>] ? verify_iovec+0x53/0x100
[  608.056705]  [<ffffffff8140abb6>] ___sys_sendmsg+0x416/0x420
[  608.056710]  [<ffffffff81151af6>] ? __do_fault+0x216/0x510
[  608.056715]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056720]  [<ffffffff81095c9e>] ? up_read+0x1e/0x40
[  608.056724]  [<ffffffff814eaf74>] ? __do_page_fault+0x2b4/0x4a0
[  608.056729]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056733]  [<ffffffff8140fddc>] ? release_sock+0x8c/0xa0
[  608.056741]  [<ffffffff8109bb5d>] ? trace_hardirqs_on+0xd/0x10
[  608.056746]  [<ffffffff81051875>] ? local_bh_enable_ip+0x65/0xc0
[  608.056751]  [<ffffffff8140ad94>] __sys_sendmsg+0x44/0x80
[  608.056756]  [<ffffffff8140ade4>] SyS_sendmsg+0x14/0x20
[  608.056761]  [<ffffffff814ef5a2>] system_call_fastpath+0x16/0x1b
-------------------------------------
Quoted 8 lines
The reason is: In test_autoclose set the autoclose to 5 not 0(default). So when we init the association, the (sctp_sock) sp->autoclose is not 0 while asoc's timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] is 0.
So when we process the COOKIE_ACK, the sctp_sf_do_5_1E_ca will be called, that will do that:
	if (asoc->autoclose) //asoc->autoclose is equal sp->autoclose
		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
this looks like a bug in how the timeout is being set. The timeout should be based on the association value, not some sysctl value.
The typical socket option values go like this:
socket value = starts at sysctl, changed by user. assoc value = starts at socket value, may be changed by user.
any timer = starts at assoc value.
Quoted 8 lines
Quoted 2 lines
this seems to be broken for autoclose.
Well, it is, but I think its a reasonable fix for the issue. As I understand the description, the problem is that if autoclose is non-zero, and the default timeout is zero, then you'll get a BUG_ON halt because you need to set a timer that will expire immediately. It seems reasonable to me to make a minimal default for the timeout to be non-zero, since it makes no sense to have a zero auto-close timeout.
Quoted 5 lines
Yes, the default timeout is zero because the timeout is latched by the sysctl value while the association value is not. So, the association value is now out-of-sync with what the system does and we end up lying to the user when they do a getsockopt().
Quoted 6 lines
Thats not true, looking at the sctp_association_init code the timeout is defaulted to the minimum of the socket value and the maxium timeout sysctl value, the latter of which is defined as INT_MAX/HZ. That seems reasonable to me in that an association latches both the timeout value and the enabled value from its parent socket at association init time (assoc->autoclose is assigned from sp->autoclose farther down in sctp_association_init).
Quoted 6 lines
Right, so let's see what happens. Let the sysctl value be set to 1. The the socket is created and the option is used to set the value to 10. Now, association is created and the timeout is latched to 1. However, the autoclose value of the association is set to 10. So now we are lying to the user about our autoclose value!
Quoted 10 lines
I think thats not really a problem here. The sysctl in question is designed to do exactly what you describe, and in your scenario above the user has change the sysctl from the default setting of MAX_INT/HZ to 1, so they know that the autocolse timeout is going to be limited. I agree that the fact that getsockopt is going to return the value 10 instead of 1 is an irritant, but its never guaranteed to match what an associations actual timeout is, because, as you note, its latched at the time the association is created. Its no different than setting SOCK_AUTOCLOSE to 10, creating an association, then changing SOCK_AUTOCLOSE to 20. The getsockopt operation will return 20 even though the existing association isn't using that as a timeout value.
Yes, the above is absolutely true for multiple calls to set autoclose. However, for a single call to autoclose, we should return the value currently in use, not the value that use the set and we may be ignoring completely.
Quoted 13 lines
Quoted 13 lines
This is what's causing the problem. The timeout value is out of sync what what the requested timeout value is.
Now, if we latch the socket value correctly, then everything just starts to work, even for sysctl value of 0, and we stop lying to the user on getsockopt() calls. This is the real bug that causing the BUG_ON.
Quoted 5 lines
Quoted 5 lines
If we simply latch the socket value properly, then we would have to worry about this at all. If the socket value is latched at 0, then autoclose is disabled.
Quoted 6 lines
Ok, so whats "correct" here? Are you proposing that we remove the max_autoclose sysctl entirely, or shall we check the value passed into the autoclose setsockopt against the sysctl value, to ensure that users don't set a value larger than what the max specifies, lest we continue to 'lie' to them by letting them set a autoclose timeout larger than what we intend to set when we initalize an association?
Yes, that's what we should do. Essentially treat this similar to how the stack SO_{SND|RCV}BUF. Might even log a warning to say autoclose is limited by sysctl.
Quoted 4 lines
Regardless of what we do here, Theres still no guarantee that
the socket level value will match any given association, and thats just the way it goes.
That's ok, as long as it uses a valid value and the association timer is set based on the value in the socket.
Quoted 8 lines
What we should do, regardless, is remove assoc->autoclose entirely, and replace all references to it with references to asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]
That way we use a single canonical place to test the autoclose value (since assoc->autoclose is only used as a boolean test anyway). Writing it down and reading it back to myself, that may actually be congruent with what you mean when you say that we should latch the socket value properly.

Quoted 6 lines

That's fine to. So we would have a socket value that is properly limited by sysctl and returns to the user the value new association will use. We would also have the timer value in the association that is initialized with the socket value. The timer value of 0 means autoclose is disabled. Does that make sense to you?

Yeah, that makes sense, I'll write it in the AM.
Neil

Quoted 31 lines

-vlad
Quoted 27 lines
Neil
Quoted 25 lines
Quoted 7 lines
We are latching the value properly from what I can see. The problem is that the upper bound on the value we latch can be zero. The only way this bug can occur is if net->sctp.max_autoclose is 0. Note that I'm not talking about the timeout value itself, but its upper bound. We shouldn't be allowed to set the autoclose timeout sysctl range to [0..0], it just doesn't make sense. It should be at least [0..1], and the socket autoclose value can range properly within that. From what I see, thats what this patch does.
We don't allow the sysctl range of [0..0]. We allow the range of [0..max_autoclose_max]. This patch tries to change the range to [1..max_autoclose_max]. This may be a correct thing to do, but it is another issue. That fact this change fixes the BUG_ON is a side-effect. The real issue is in how we set set the timer value.
-vlad
Quoted 3 lines
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2013-12-06 15:29 UTC #11 lore raw

PATCH 1/1 sctp: properly latch and use autoclose value from sock to association

Currently, sctp associations latch a sockets autoclose value to an association at association init time, subject to capping constraints from the max_autoclose sysctl value. This leads to an odd situation where an application may set a socket level autoclose timeout, but sliently sctp will limit the autoclose timeout to something less than that.

Fix this by modifying the autoclose setsockopt function to check the limit, cap it and warn the user via syslog that the timeout is capped. This will allow getsockopt to return valid autoclose timeout values that reflect what subsequent associations actually use.

While were at it, also elimintate the assoc->autoclose variable, it duplicates whats in the timeout array, which leads to multiple sources for the same information, that may differ (as the former isn't subject to any capping). This gives us the timeout information in a canonical place and saves some space in the association structure as well.

Signed-off-by Neil Horman <[email protected]>

CC Wang Weidong <[email protected]>

CC David Miller <[email protected]>

CC Vlad Yasevich <[email protected]>

---
 include/net/sctp/structs.h |  6 ------
 net/sctp/associola.c       |  2 --
 net/sctp/output.c          |  3 ++-
 net/sctp/sm_statefuns.c    | 12 ++++++------
 net/sctp/socket.c          |  7 +++++++
 5 files changed, 15 insertions(+), 15 deletions(-)

include/net/sctp/structs.h +0 -6

index 2174d8d..0b069a0 100644
@@ -1725,12 +1725,6 @@ struct sctp_association {
 	/* How many duplicated TSNs have we seen?  */
 	int numduptsns;
 
-	/* Number of seconds of idle time before an association is closed.
-	 * In the association context, this is really used as a boolean
-	 * since the real timeout is stored in the timeouts array
-	 */
-	__u32 autoclose;
-
 	/* These are to support
 	 * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
 	 *  and Enforcement of Flow and Message Limits"

net/sctp/associola.c +0 -2

index c9b91cb..0c9d1f1 100644
@@ -291,8 +291,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		asoc->peer.ipv6_address = 1;
 	INIT_LIST_HEAD(&asoc->asocs);
 
-	asoc->autoclose = sp->autoclose;
-
 	asoc->default_stream = sp->default_stream;
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;

net/sctp/output.c +2 -1

index e650978..c89e57d 100644
@@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 		unsigned long timeout;
 
 		/* Restart the AUTOCLOSE timer when sending data. */
-		if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
+		if (sctp_state(asoc, ESTABLISHED) &&
+		    asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 			timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
 			timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];

net/sctp/sm_statefuns.c +6 -6

index dfe3f36..a26065b 100644
@@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	if (new_asoc->autoclose)
+	if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
 	if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
 		force = SCTP_FORCE();
 
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
 				SCTP_CHUNK(chunk));
 
 	/* Count this as receiving DATA. */
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

net/sctp/socket.c +7 -0

index 72046b9..9486ffd 100644
@@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 				     unsigned int optlen)
 {
 	struct sctp_sock *sp = sctp_sk(sk);
+	struct net *net = sock_net(sk);
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
+
 	return 0;
 }

Signature · 1 line

1.8.3.1

Vlad Yasevich

2013-12-06 15:36 UTC #12 lore raw

Re: [PATCH] sctp: properly latch and use autoclose value from sock to association

Quoted 148 lines · Neil Horman

Signed-off-by Neil Horman <[email protected]>

CC Wang Weidong <[email protected]>

CC David Miller <[email protected]>

CC Vlad Yasevich <[email protected]>

---
 include/net/sctp/structs.h |  6 ------
 net/sctp/associola.c       |  2 --
 net/sctp/output.c          |  3 ++-
 net/sctp/sm_statefuns.c    | 12 ++++++------
 net/sctp/socket.c          |  7 +++++++
 5 files changed, 15 insertions(+), 15 deletions(-)

include/net/sctp/structs.h +0 -6

index 2174d8d..0b069a0 100644
@@ -1725,12 +1725,6 @@ struct sctp_association {
 	/* How many duplicated TSNs have we seen?  */
 	int numduptsns;
 
-	/* Number of seconds of idle time before an association is closed.
-	 * In the association context, this is really used as a boolean
-	 * since the real timeout is stored in the timeouts array
-	 */
-	__u32 autoclose;
-
 	/* These are to support
 	 * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
 	 *  and Enforcement of Flow and Message Limits"

net/sctp/associola.c +0 -2

index c9b91cb..0c9d1f1 100644
@@ -291,8 +291,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		asoc->peer.ipv6_address = 1;
 	INIT_LIST_HEAD(&asoc->asocs);
 
-	asoc->autoclose = sp->autoclose;
-
 	asoc->default_stream = sp->default_stream;
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;

net/sctp/output.c +2 -1

index e650978..c89e57d 100644
@@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 		unsigned long timeout;
 
 		/* Restart the AUTOCLOSE timer when sending data. */
-		if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
+		if (sctp_state(asoc, ESTABLISHED) &&
+		    asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 			timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
 			timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];

net/sctp/sm_statefuns.c +6 -6

index dfe3f36..a26065b 100644
@@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	if (new_asoc->autoclose)
+	if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
 	if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
 		force = SCTP_FORCE();
 
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
 				SCTP_CHUNK(chunk));
 
 	/* Count this as receiving DATA. */
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

net/sctp/socket.c +1 -0

index 72046b9..9486ffd 100644
@@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 				     unsigned int optlen)
 {
 	struct sctp_sock *sp = sctp_sk(sk);
+	struct net *net = sock_net(sk);
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;

net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2217

Quoted 10 lines

net/sctp/socket.c +6 -0

 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
+
 	return 0;
 }

Now that we do this, we can do this as well:

net/sctp/associola.c +0 -0

index 68a27f9..7e86957 100644
@@ -154,8 +154,7 @@ static struct sctp_association

*sctp_association_init(struct sctp_association *a

        asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
        asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;

- asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] - min_t(unsigned long, sp->autoclose,
net->sctp.max_autoclose) * HZ;
+ asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;

-vlad

David Laight

2013-12-06 15:46 UTC #19 lore raw

RE: [PATCH] sctp: properly latch and use autoclose value from sock to association

Quoted 11 lines

From: Neil Horman
Currently, sctp associations latch a sockets autoclose value to an association at association init time, subject to capping constraints from the max_autoclose sysctl value. This leads to an odd situation where an application may set a socket level autoclose timeout, but sliently sctp will limit the autoclose timeout to something less than that.
Fix this by modifying the autoclose setsockopt function to check the limit, cap it and warn the user via syslog that the timeout is capped. This will allow getsockopt to return valid autoclose timeout values that reflect what subsequent associations actually use.

...

net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2213

Quoted 5 lines

net/sctp/socket.c +5 -0

+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}

Is it really reasonable to allow a user to spam syslog this way?

David

Neil Horman

2013-12-06 15:48 UTC #13 lore raw

Re: [PATCH] sctp: properly latch and use autoclose value from sock to association

Quoted 166 lines · Vlad Yasevich

Quoted 148 lines · Neil Horman

Signed-off-by Neil Horman <[email protected]>

CC Wang Weidong <[email protected]>

CC David Miller <[email protected]>

CC Vlad Yasevich <[email protected]>

---
 include/net/sctp/structs.h |  6 ------
 net/sctp/associola.c       |  2 --
 net/sctp/output.c          |  3 ++-
 net/sctp/sm_statefuns.c    | 12 ++++++------
 net/sctp/socket.c          |  7 +++++++
 5 files changed, 15 insertions(+), 15 deletions(-)

include/net/sctp/structs.h +0 -6

index 2174d8d..0b069a0 100644
@@ -1725,12 +1725,6 @@ struct sctp_association {
 	/* How many duplicated TSNs have we seen?  */
 	int numduptsns;
 
-	/* Number of seconds of idle time before an association is closed.
-	 * In the association context, this is really used as a boolean
-	 * since the real timeout is stored in the timeouts array
-	 */
-	__u32 autoclose;
-
 	/* These are to support
 	 * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
 	 *  and Enforcement of Flow and Message Limits"

net/sctp/associola.c +0 -2

index c9b91cb..0c9d1f1 100644
@@ -291,8 +291,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		asoc->peer.ipv6_address = 1;
 	INIT_LIST_HEAD(&asoc->asocs);
 
-	asoc->autoclose = sp->autoclose;
-
 	asoc->default_stream = sp->default_stream;
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;

net/sctp/output.c +2 -1

index e650978..c89e57d 100644
@@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 		unsigned long timeout;
 
 		/* Restart the AUTOCLOSE timer when sending data. */
-		if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
+		if (sctp_state(asoc, ESTABLISHED) &&
+		    asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 			timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
 			timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];

net/sctp/sm_statefuns.c +6 -6

index dfe3f36..a26065b 100644
@@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	if (new_asoc->autoclose)
+	if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
 	if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
 		force = SCTP_FORCE();
 
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
 				SCTP_CHUNK(chunk));
 
 	/* Count this as receiving DATA. */
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

net/sctp/socket.c +1 -0

index 72046b9..9486ffd 100644
@@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 				     unsigned int optlen)
 {
 	struct sctp_sock *sp = sctp_sk(sk);
+	struct net *net = sock_net(sk);
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;

net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2217

Quoted 10 lines

net/sctp/socket.c +6 -0

 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
+
 	return 0;
 }

Now that we do this, we can do this as well:

net/sctp/associola.c +0 -0

index 68a27f9..7e86957 100644

net/sctp/associola.c static struct sctp_association · line 160

Quoted 8 lines

net/sctp/associola.c +1 -1

@@ -154,8 +154,7 @@ static struct sctp_association
 *sctp_association_init(struct sctp_association *a
 
        asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
        asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
-       asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > -               min_t(unsigned long, sp->autoclose,
 net->sctp.max_autoclose) * HZ;
+       asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;

I actually kept that out of the patch specifically because I was concerned about the case where we set the autoclose socket option, then reduce the max_autoclose value. If we do this, then we have an unbounded race in which the old autoclose value may be applied above the cap. Do you still want to put it in?

Neil

Quoted 7 lines

net/sctp/associola.c +0 -2
-vlad
 
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2013-12-06 16:04 UTC #20 lore raw

Re: [PATCH] sctp: properly latch and use autoclose value from sock to association

In reply to David Laight

Quoted 20 lines · David Laight

Quoted 11 lines
From: Neil Horman
Currently, sctp associations latch a sockets autoclose value to an association at association init time, subject to capping constraints from the max_autoclose sysctl value. This leads to an odd situation where an application may set a socket level autoclose timeout, but sliently sctp will limit the autoclose timeout to something less than that.
Fix this by modifying the autoclose setsockopt function to check the limit, cap it and warn the user via syslog that the timeout is capped. This will allow getsockopt to return valid autoclose timeout values that reflect what subsequent associations actually use.
...
net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2213
Quoted 5 lines
net/sctp/socket.c +5 -0
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
Is it really reasonable to allow a user to spam syslog this way?

Quoted 2 lines

David

Hmm, good point, maybe a pr_ratelimit gate?

Neil

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2013-12-06 16:29 UTC #21 lore raw

Re: [PATCH] sctp: properly latch and use autoclose value from sock to association

In reply to David Laight

Quoted 20 lines · David Laight

Quoted 11 lines
From: Neil Horman
Currently, sctp associations latch a sockets autoclose value to an association at association init time, subject to capping constraints from the max_autoclose sysctl value. This leads to an odd situation where an application may set a socket level autoclose timeout, but sliently sctp will limit the autoclose timeout to something less than that.
Fix this by modifying the autoclose setsockopt function to check the limit, cap it and warn the user via syslog that the timeout is capped. This will allow getsockopt to return valid autoclose timeout values that reflect what subsequent associations actually use.
...
net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2213
Quoted 5 lines
net/sctp/socket.c +5 -0
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
Is it really reasonable to allow a user to spam syslog this way?

Quoted 3 lines

David

Actually, I think its ok, or at least common. Theres lots of pr_messages checking common user errors in the various setockopt paths. We should probably undertake a larger effort to convert those all to printk_ratelimited with some level of sctp shared state.
Neil

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Horman

2013-12-06 17:22 UTC #14 lore raw

PATCH v2 1/1 sctp: properly latch and use autoclose value from sock to association

Signed-off-by Neil Horman <[email protected]>

CC Wang Weidong <[email protected]>

CC David Miller <[email protected]>

CC Vlad Yasevich <[email protected]>

---
Change notes:

V2) Based on conversation with Vlad, removing the cap operation from sctp_association_init. The spec is vague as to how we should handle this, and so we're removing it as a code savings, and will readdress if anyone complains.

---
 include/net/sctp/structs.h |  6 ------
 net/sctp/associola.c       |  5 +----
 net/sctp/output.c          |  3 ++-
 net/sctp/sm_statefuns.c    | 12 ++++++------
 net/sctp/socket.c          |  7 +++++++
 5 files changed, 16 insertions(+), 17 deletions(-)

include/net/sctp/structs.h +0 -6

index 2174d8d..0b069a0 100644
@@ -1725,12 +1725,6 @@ struct sctp_association {
 	/* How many duplicated TSNs have we seen?  */
 	int numduptsns;
 
-	/* Number of seconds of idle time before an association is closed.
-	 * In the association context, this is really used as a boolean
-	 * since the real timeout is stored in the timeouts array
-	 */
-	__u32 autoclose;
-
 	/* These are to support
 	 * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
 	 *  and Enforcement of Flow and Message Limits"

net/sctp/associola.c +1 -3

index c9b91cb..be939b1 100644
@@ -154,8 +154,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] -		min_t(unsigned long, sp->autoclose, net->sctp.max_autoclose) * HZ;
+	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
 
 	/* Initializes the timers */
 	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)

@@ -291,8 +290,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		asoc->peer.ipv6_address = 1;
 	INIT_LIST_HEAD(&asoc->asocs);
 
-	asoc->autoclose = sp->autoclose;
-
 	asoc->default_stream = sp->default_stream;
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;

net/sctp/output.c +2 -1

index e650978..c89e57d 100644
@@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 		unsigned long timeout;
 
 		/* Restart the AUTOCLOSE timer when sending data. */
-		if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
+		if (sctp_state(asoc, ESTABLISHED) &&
+		    asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 			timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
 			timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];

net/sctp/sm_statefuns.c +6 -6

index dfe3f36..a26065b 100644
@@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	if (new_asoc->autoclose)
+	if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
 	if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
 		force = SCTP_FORCE();
 
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
 				SCTP_CHUNK(chunk));
 
 	/* Count this as receiving DATA. */
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

net/sctp/socket.c +7 -0

index 72046b9..9486ffd 100644
@@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 				     unsigned int optlen)
 {
 	struct sctp_sock *sp = sctp_sk(sk);
+	struct net *net = sock_net(sk);
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
+
 	return 0;
 }

Signature · 1 line

1.8.3.1

Vlad Yasevich

Acked-by on v2 1/1

2013-12-06 18:47 UTC #15 lore raw

Re: [PATCH v2] sctp: properly latch and use autoclose value from sock to association

Quoted 23 lines · Neil Horman

Currently, sctp associations latch a sockets autoclose value to an association at association init time, subject to capping constraints from the max_autoclose sysctl value. This leads to an odd situation where an application may set a socket level autoclose timeout, but sliently sctp will limit the autoclose timeout to something less than that.
Fix this by modifying the autoclose setsockopt function to check the limit, cap it and warn the user via syslog that the timeout is capped. This will allow getsockopt to return valid autoclose timeout values that reflect what subsequent associations actually use.
While were at it, also elimintate the assoc->autoclose variable, it duplicates whats in the timeout array, which leads to multiple sources for the same information, that may differ (as the former isn't subject to any capping). This gives us the timeout information in a canonical place and saves some space in the association structure as well.
Signed-off-by Neil Horman <[email protected]>
CC Wang Weidong <[email protected]>
CC David Miller <[email protected]>
CC Vlad Yasevich <[email protected]>
CC [email protected]

Quoted 6 lines

---
Change notes:
V2) Based on conversation with Vlad, removing the cap operation from sctp_association_init. The spec is vague as to how we should handle this, and so we're removing it as a code savings, and will readdress if anyone complains.

This is makes this behavior consistent with the rest of the timers that SCTP uses, i.e. the association timer is latched to the current socket value.

Acked-by Vlad Yasevich <[email protected]>

-vlad

Quoted 145 lines

---
 include/net/sctp/structs.h |  6 ------
 net/sctp/associola.c       |  5 +----
 net/sctp/output.c          |  3 ++-
 net/sctp/sm_statefuns.c    | 12 ++++++------
 net/sctp/socket.c          |  7 +++++++
 5 files changed, 16 insertions(+), 17 deletions(-)

include/net/sctp/structs.h +0 -6

index 2174d8d..0b069a0 100644
@@ -1725,12 +1725,6 @@ struct sctp_association {
 	/* How many duplicated TSNs have we seen?  */
 	int numduptsns;
 
-	/* Number of seconds of idle time before an association is closed.
-	 * In the association context, this is really used as a boolean
-	 * since the real timeout is stored in the timeouts array
-	 */
-	__u32 autoclose;
-
 	/* These are to support
 	 * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
 	 *  and Enforcement of Flow and Message Limits"

net/sctp/associola.c +1 -3

index c9b91cb..be939b1 100644
@@ -154,8 +154,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > -		min_t(unsigned long, sp->autoclose, net->sctp.max_autoclose) * HZ;
+	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
 
 	/* Initializes the timers */
 	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)

@@ -291,8 +290,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		asoc->peer.ipv6_address = 1;
 	INIT_LIST_HEAD(&asoc->asocs);
 
-	asoc->autoclose = sp->autoclose;
-
 	asoc->default_stream = sp->default_stream;
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;

net/sctp/output.c +2 -1

index e650978..c89e57d 100644
@@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 		unsigned long timeout;
 
 		/* Restart the AUTOCLOSE timer when sending data. */
-		if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
+		if (sctp_state(asoc, ESTABLISHED) &&
+		    asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 			timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
 			timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];

net/sctp/sm_statefuns.c +6 -6

index dfe3f36..a26065b 100644
@@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	if (new_asoc->autoclose)
+	if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
 	if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
 		force = SCTP_FORCE();
 
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
 				SCTP_CHUNK(chunk));
 
 	/* Count this as receiving DATA. */
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

net/sctp/socket.c +7 -0

index 72046b9..9486ffd 100644
@@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 				     unsigned int optlen)
 {
 	struct sctp_sock *sp = sctp_sk(sk);
+	struct net *net = sock_net(sk);
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
+
 	return 0;
 }

Wang Weidong

author Acked-by on v2 1/1

2013-12-07 06:29 UTC #16 lore raw

Re: [PATCH v2] sctp: properly latch and use autoclose value from sock to association

Quoted 164 lines · Neil Horman

Signed-off-by Neil Horman <[email protected]>

CC Wang Weidong <[email protected]>

CC David Miller <[email protected]>

CC Vlad Yasevich <[email protected]>

---
Change notes:

---
 include/net/sctp/structs.h |  6 ------
 net/sctp/associola.c       |  5 +----
 net/sctp/output.c          |  3 ++-
 net/sctp/sm_statefuns.c    | 12 ++++++------
 net/sctp/socket.c          |  7 +++++++
 5 files changed, 16 insertions(+), 17 deletions(-)

include/net/sctp/structs.h +0 -6

index 2174d8d..0b069a0 100644
@@ -1725,12 +1725,6 @@ struct sctp_association {
 	/* How many duplicated TSNs have we seen?  */
 	int numduptsns;
 
-	/* Number of seconds of idle time before an association is closed.
-	 * In the association context, this is really used as a boolean
-	 * since the real timeout is stored in the timeouts array
-	 */
-	__u32 autoclose;
-
 	/* These are to support
 	 * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
 	 *  and Enforcement of Flow and Message Limits"

net/sctp/associola.c +1 -3

index c9b91cb..be939b1 100644
@@ -154,8 +154,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] > -		min_t(unsigned long, sp->autoclose, net->sctp.max_autoclose) * HZ;
+	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
 
 	/* Initializes the timers */
 	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)

@@ -291,8 +290,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		asoc->peer.ipv6_address = 1;
 	INIT_LIST_HEAD(&asoc->asocs);
 
-	asoc->autoclose = sp->autoclose;
-
 	asoc->default_stream = sp->default_stream;
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;

net/sctp/output.c +2 -1

index e650978..c89e57d 100644
@@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 		unsigned long timeout;
 
 		/* Restart the AUTOCLOSE timer when sending data. */
-		if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
+		if (sctp_state(asoc, ESTABLISHED) &&
+		    asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 			timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
 			timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];

net/sctp/sm_statefuns.c +6 -6

index dfe3f36..a26065b 100644
@@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	if (new_asoc->autoclose)
+	if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
 	if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
 		force = SCTP_FORCE();
 
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
 				SCTP_CHUNK(chunk));
 
 	/* Count this as receiving DATA. */
-	if (asoc->autoclose) {
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
 	}

@@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

@@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
 	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
 
-	if (asoc->autoclose)
+	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));

net/sctp/socket.c +1 -0

index 72046b9..9486ffd 100644
@@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 				     unsigned int optlen)
 {
 	struct sctp_sock *sp = sctp_sk(sk);
+	struct net *net = sock_net(sk);
 
 	/* Applicable to UDP-style socket only */
 	if (sctp_style(sk, TCP))

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;

net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2217

Quoted 10 lines

net/sctp/socket.c +6 -0

 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}
+
 	return 0;
 }

I test it on my machine. And it works for me.
Thanks.

Acked-by Wang Weidong <[email protected]>

David Miller

maintainer

2013-12-10 01:32 UTC #22 lore raw

Re: [PATCH] sctp: properly latch and use autoclose value from sock to association

From: Neil Horman <[email protected]>
Date: Fri, 6 Dec 2013 10:29:15 -0500

net/sctp/socket.c static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, · line 2213

Quoted 9 lines

net/sctp/socket.c +5 -0

@@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
 
+	if (sp->autoclose > net->sctp.max_autoclose) {
+		pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
+			sp->autoclose, net->sctp.max_autoclose);
+		sp->autoclose = net->sctp.max_autoclose;
+	}

Please don't add this warning.

If we already do this for certain setsockopt calls in SCTP, that just means we have bugs to fix.

Neil Horman

2013-12-10 11:48 UTC #17 lore raw

PATCH v3 1/1 sctp: properly latch and use autoclose value from sock to association

Signed-off-by Neil Horman <[email protected]>

Acked-by Vlad Yasevich <[email protected]>

CC Wang Weidong <[email protected]>

CC David Miller <[email protected]>

CC Vlad Yasevich <[email protected]>