Re: [PATCH] sctp: Fix error handling for switch statement case in the function sctp_cmd_interprete
On 2016-04-05 07:29 PM, David Miller wrote: > From: Daniel Borkmann > Date: Tue, 05 Apr 2016 23:53:52 +0200 > >> On 04/05/2016 11:36 PM, Bastien Philbert wrote: >>> This fixes error handling for the switch statement case >>> SCTP_CMD_SEND_PKT by making the error value of the call >>> to sctp_packet_transmit equal the variable error due to >>> this function being able to fail with a error code. In >> >> What actual issue have you observed that you fix? >> >>> addition allow the call to sctp_ootb_pkt_free afterwards >>> to free up the no longer in use sctp packet even if the >>> call to the function sctp_packet_transmit fails in order >>> to avoid a memory leak here for not freeing the sctp >> >> Not sure how this relates to your code? > > Bastien, I'm seeing a clear negative pattern with the bug fixes > you are submitting. > > Just now you submitted the ICMP change which obviously was never > tested because it tried to take the RTNL mutex in atomic context, > and now this sctp thing. > > If you don't start actually testing your changes and expalining > clearly what the problem actually is, how you discovered it, > and how you actually tested your patch, I will start completely > ignoring your patch submissions. > Ok sure I will be more careful with my future patches. Sorry about those two patches :(. Bastien
Re: [PATCH] sctp: Fix error handling for switch statement case in the function sctp_cmd_interprete
On 2016-04-05 06:12 PM, Marcelo Ricardo Leitner wrote: > On Tue, Apr 05, 2016 at 05:36:41PM -0400, Bastien Philbert wrote: >> This fixes error handling for the switch statement case >> SCTP_CMD_SEND_PKT by making the error value of the call >> to sctp_packet_transmit equal the variable error due to >> this function being able to fail with a error code. In >> addition allow the call to sctp_ootb_pkt_free afterwards >> to free up the no longer in use sctp packet even if the >> call to the function sctp_packet_transmit fails in order >> to avoid a memory leak here for not freeing the sctp > > This leak shouldn't exist as sctp_packet_transmit() will free the packet > if it returns ENOMEM, through the nomem: handling. > > But about making it visible to the user, that looks interesting to me > although I cannot foresee yet its effects, like the comment at the end > of sctp_packet_transmit() on not returning EHOSTUNREACH. Did you check > it? > I was aware of the -EHOSTUNREACH issue but assumed that this needs to be known to functions internal to the kernel. TO rephase does it matter if the callers of this function known if sctp_packet_transmit or care if it fails or is this just unnecessary as we do cleanup else where which is enough so the new error check is not needed? Again if their is a certain test would like me to run on this patch too to make sure it's OK I don't mind, just let me known :). Cheers, Bastien >> >> Signed-off-by: Bastien Philbert >> --- >> net/sctp/sm_sideeffect.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c >> index 7fe56d0..f3a8b58 100644 >> --- a/net/sctp/sm_sideeffect.c >> +++ b/net/sctp/sm_sideeffect.c >> @@ -1434,7 +1434,7 @@ static int sctp_cmd_interpreter(sctp_event_t >> event_type, >> case SCTP_CMD_SEND_PKT: >> /* Send a full packet to our peer. */ >> packet = cmd->obj.packet; >> -sctp_packet_transmit(packet, gfp); >> +error = sctp_packet_transmit(packet, gfp); >> sctp_ootb_pkt_free(packet); >> break; >> >> -- >> 2.5.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
Re: [PATCH] sctp: Fix error handling for switch statement case in the function sctp_cmd_interprete
On 2016-04-05 05:53 PM, Daniel Borkmann wrote: > On 04/05/2016 11:36 PM, Bastien Philbert wrote: >> This fixes error handling for the switch statement case >> SCTP_CMD_SEND_PKT by making the error value of the call >> to sctp_packet_transmit equal the variable error due to >> this function being able to fail with a error code. In > > What actual issue have you observed that you fix? > The issue here is basically that sctp_packet_transmit can return a error if it unsuccessfully transmit the sk_buff as a parameter. Seems that we should signal the user/caller(s) when a sctp packet transmission fails here. If you would like I can resend with a better commit message in a V2 if this explains the issue better. Bastien >> addition allow the call to sctp_ootb_pkt_free afterwards >> to free up the no longer in use sctp packet even if the >> call to the function sctp_packet_transmit fails in order >> to avoid a memory leak here for not freeing the sctp > > Not sure how this relates to your code? > >> Signed-off-by: Bastien Philbert >> --- >> net/sctp/sm_sideeffect.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c >> index 7fe56d0..f3a8b58 100644 >> --- a/net/sctp/sm_sideeffect.c >> +++ b/net/sctp/sm_sideeffect.c >> @@ -1434,7 +1434,7 @@ static int sctp_cmd_interpreter(sctp_event_t >> event_type, >> case SCTP_CMD_SEND_PKT: >> /* Send a full packet to our peer. */ >> packet = cmd->obj.packet; >> -sctp_packet_transmit(packet, gfp); >> +error = sctp_packet_transmit(packet, gfp); >> sctp_ootb_pkt_free(packet); >> break; >> >> >
[PATCH] sctp: Fix error handling for switch statement case in the function sctp_cmd_interprete
This fixes error handling for the switch statement case SCTP_CMD_SEND_PKT by making the error value of the call to sctp_packet_transmit equal the variable error due to this function being able to fail with a error code. In addition allow the call to sctp_ootb_pkt_free afterwards to free up the no longer in use sctp packet even if the call to the function sctp_packet_transmit fails in order to avoid a memory leak here for not freeing the sctp Signed-off-by: Bastien Philbert --- net/sctp/sm_sideeffect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index 7fe56d0..f3a8b58 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1434,7 +1434,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type, case SCTP_CMD_SEND_PKT: /* Send a full packet to our peer. */ packet = cmd->obj.packet; - sctp_packet_transmit(packet, gfp); + error = sctp_packet_transmit(packet, gfp); sctp_ootb_pkt_free(packet); break; -- 2.5.0
[PATCH] ipv6: icmp: Add protection from concurrent users in the function icmpv6_echo_reply
This adds protection from concurrenct users in the function icmpv6_echo_reply around the call to the function __in6_dev_get by locking/unlocking around this call with calls to the functions rtnl_lock and rtnl_unlock to protect against concurrent users when calling this function in icmpv6_echo_reply as stated in the comments for locking requirements for the function, __in6_dev_get. Signed-off-by: Bastien Philbert --- net/ipv6/icmp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index 0a37ddc..798434f 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -607,7 +607,9 @@ static void icmpv6_echo_reply(struct sk_buff *skb) hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst); + rtnl_lock(); idev = __in6_dev_get(skb->dev); + rtnl_unlock(); msg.skb = skb; msg.offset = 0; -- 2.5.0
[PATCH] ipsec: Fix error handling in the function xfrm6_get_addr
This fixes the error handling in the function xfrm_get_addr by
checking if the call to ipv6_dev_get_saddr has failed and if
so return -EADDRNOTAVAIL to signal to the caller of xfrm6_get_addr
that the call has failed the error should be handled by the caller
while also freeing the dst_entry structure pointer in use by this
Signed-off-by: Bastien Philbert
---
net/ipv6/xfrm6_policy.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index c074771..759473e 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -64,7 +64,10 @@ static int xfrm6_get_saddr(struct net *net, int oif,
return -EHOSTUNREACH;
dev = ip6_dst_idev(dst)->dev;
- ipv6_dev_get_saddr(dev_net(dev), dev, &daddr->in6, 0, &saddr->in6);
+ if (ipv6_dev_get_saddr(dev_net(dev), dev, &daddr->in6, 0, &saddr->in6))
{
+ dst_release(dst);
+ return -EADDRNOTAVAIL;
+ }
dst_release(dst);
return 0;
}
--
2.5.0
[PATCH] bluetooth: Fix locking issues in the function l2cap_connect_cfm
This fixes a locking issue in the function l2cap_connect_cfm for not locking the mutex lock for channels on the l2cap_conn structure pointer conn before calling __l2cap_get_chan_by_dcid as all callers need to lock and unlock this mutex before calling this function due to issues with either concurrent users or race conditions arising Signed-off-by: Bastien Philbert --- net/bluetooth/l2cap_core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index eb4f5f2..2ab103e 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -7308,6 +7308,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) struct l2cap_chan *chan, *next; /* Client fixed channels should override server ones */ + mutex_lock(&conn->chan_lock); if (__l2cap_get_chan_by_dcid(conn, pchan->scid)) goto next; @@ -7324,6 +7325,7 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status) l2cap_chan_unlock(pchan); next: + mutex_unlock(&conn->chan_lock); next = l2cap_global_fixed_chan(pchan, hcon); l2cap_chan_put(pchan); pchan = next; -- 2.5.0
Re: [PATCH] bridge:Fix incorrect variable assignment on error path in br_sysfs_addbr
On 2016-04-04 04:14 PM, David Miller wrote: > From: Bastien Philbert > Date: Sun, 3 Apr 2016 19:04:26 -0400 > >> This fixes the incorrect variable assignment on error path in >> br_sysfs_addbr for when the call to kobject_create_and_add >> fails to assign the value of -EINVAL to the returned variable of >> err rather then incorrectly return zero making callers think this >> function has succeededed due to the previous assignment being >> assigned zero when assigning it the successful return value of >> the call to sysfs_create_group which is zero. >> >> Signed-off-by: Bastien Philbert > > Applied, but please put a space after the subsystem prefix and the > colon character in your subject lines in the future. > > Doesn't that really look odd to you, the way you did it? "net:Fix"? > > Doesn't it look more natural, and consistent with what all other > patch submitters do, if it's "net: Fix"? > > Thanks. > Done, sorry about that :(. Will remember for future patches. Bastien
Re: System hangs (unable to handle kernel paging request)
On 2016-04-04 11:01 AM, Oleksii Berezhniak wrote: > Can you please point me to more detailed description of similar issues > that you mentioned? > Mostly it's in reworks for the Intel Drivers related to improving performance in order to avoid over usage of CPU leading to a soft lockup being found during kernel polling at high loads with millions of packets being send per second. In addition this has been in various parts of these drivers so it's hard to find one exact detailed commit. However I based my finding of this commit maybe helping you based on the release history of the longterm kernel your using as the release date for that commit is way after your kernel was released. However you may want to check if the commit with the id I sent you has been back ported to your kernel, if so and this is being *still* triggered then this is probably a bug somewhere else. Cheers, Bastien > I can only find this: > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=32b3e08fff60494cd1d281a39b51583edfd2b18f > > But there are no any hangs. Only performance issues. > > BTW, GRO (Generic Receive Offloading) is disabled on our network adapter. > > 2016-04-04 17:30 GMT+03:00 Bastien Philbert : >> >> >> On 2016-04-04 03:59 AM, Oleksii Berezhniak wrote: >>> Good day. >>> >>> We have PPPoE server with CentOS 7 (kernel 3.10.0-327.10.1.el7.dsip.x86_64) >>> >>> We applied some PPPoE related patches to this kernel: >>> >>> ppp: don't override sk->sk_state in pppoe_flush_dev() >>> ppp: fix pppoe_dev deletion condition in pppoe_release() >>> pppoe: fix memory corruption in padt work structure >>> pppoe: fix reference counting in PPPoE proxy >>> >>> Also we built latest version of ixgbe driver from Intel. >>> >>> Now we have crashes after approx. one week of uptime: >>> >>> [545444.673270] BUG: unable to handle kernel paging request at >>> 88a005040200 >>> [545444.673306] IP: [] kmem_cache_alloc+0x75/0x1d0 >>> [545444.673335] PGD 0 >>> [545444.673348] Oops: [#1] SMP >>> [545444.673367] Modules linked in: arc4 ppp_mppe act_police cls_u32 >>> sch_ingress sch_tbf pptp gre pppoe pppox ppp_generic slhc 8021q garp >>> stp mrp llc iptable_nat nf_conn >>> track_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat iptable_filter xt_TCPMSS >>> iptable_mangle xt_CT nf_conntrack iptable_raw w83793 hwmon_vid >>> snd_hda_codec_realtek snd_hda_codec >>> _generic snd_hda_intel snd_hda_codec coretemp snd_hda_core iTCO_wdt >>> kvm iTCO_vendor_support snd_hwdep snd_seq snd_seq_device ipmi_ssif >>> ppdev lpc_ich snd_pcm pcspkr mfd_ >>> core sg ipmi_si snd_timer snd i2c_i801 ipmi_msghandler ioatdma >>> parport_pc parport shpchp soundcore i7core_edac tpm_infineon edac_core >>> ip_tables ext4 mbcache jbd2 sd_mod >>> crct10dif_generic crc_t10dif crct10dif_common syscopyarea sysfillrect >>> firewire_ohci sysimgblt i2c_algo_bit drm_kms_helper ata_generic >>> pata_acpi >>> [545444.674383] ttm firewire_core crc_itu_t serio_raw drm ata_piix >>> libata crc32c_intel i2c_core ixgbe(OE) vxlan e1000e ip6_udp_tunnel >>> udp_tunnel aacraid dca ptp pps_co >>> re >>> [545444.674783] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G OE >>> 3.10.0-327.10.1.el7.dsip.x86_64 #1 >>> [545444.675032] Hardware name: empty empty/S7010, BIOS 'V2.06 ' 03/31/2010 >>> [545444.675162] task: 880139c55c00 ti: 880139c84000 task.ti: >>> 880139c84000 >>> [545444.675400] RIP: 0010:[] [] >>> kmem_cache_alloc+0x75/0x1d0 >>> [545444.675641] RSP: 0018:88023fc23ce8 EFLAGS: 00010286 >>> [545444.675766] RAX: RBX: 8802302eab00 RCX: >>> 00010eb8edbe >>> [545444.676002] RDX: 00010eb8edbd RSI: 0020 RDI: >>> 88013b803700 >>> [545444.676237] RBP: 88023fc23d18 R08: 000175a0 R09: >>> 81517e70 >>> [545444.676472] R10: 006b R11: R12: >>> 88a005040200 >>> [545444.676706] R13: 0020 R14: 88013b803700 R15: >>> 88013b803700 >>> [545444.676942] FS: () GS:88023fc2() >>> knlGS: >>> [545444.677180] CS: 0010 DS: ES: CR0: 8005003b >>> [545444.677307] CR2: 88a005040200 CR3: 000237e63000 CR4: >>> 07e0 >>> [545444.677543] DR0: DR1: DR2: >>> >>>
Re: System hangs (unable to handle kernel paging request)
On 2016-04-04 03:59 AM, Oleksii Berezhniak wrote: > Good day. > > We have PPPoE server with CentOS 7 (kernel 3.10.0-327.10.1.el7.dsip.x86_64) > > We applied some PPPoE related patches to this kernel: > > ppp: don't override sk->sk_state in pppoe_flush_dev() > ppp: fix pppoe_dev deletion condition in pppoe_release() > pppoe: fix memory corruption in padt work structure > pppoe: fix reference counting in PPPoE proxy > > Also we built latest version of ixgbe driver from Intel. > > Now we have crashes after approx. one week of uptime: > > [545444.673270] BUG: unable to handle kernel paging request at > 88a005040200 > [545444.673306] IP: [] kmem_cache_alloc+0x75/0x1d0 > [545444.673335] PGD 0 > [545444.673348] Oops: [#1] SMP > [545444.673367] Modules linked in: arc4 ppp_mppe act_police cls_u32 > sch_ingress sch_tbf pptp gre pppoe pppox ppp_generic slhc 8021q garp > stp mrp llc iptable_nat nf_conn > track_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat iptable_filter xt_TCPMSS > iptable_mangle xt_CT nf_conntrack iptable_raw w83793 hwmon_vid > snd_hda_codec_realtek snd_hda_codec > _generic snd_hda_intel snd_hda_codec coretemp snd_hda_core iTCO_wdt > kvm iTCO_vendor_support snd_hwdep snd_seq snd_seq_device ipmi_ssif > ppdev lpc_ich snd_pcm pcspkr mfd_ > core sg ipmi_si snd_timer snd i2c_i801 ipmi_msghandler ioatdma > parport_pc parport shpchp soundcore i7core_edac tpm_infineon edac_core > ip_tables ext4 mbcache jbd2 sd_mod > crct10dif_generic crc_t10dif crct10dif_common syscopyarea sysfillrect > firewire_ohci sysimgblt i2c_algo_bit drm_kms_helper ata_generic > pata_acpi > [545444.674383] ttm firewire_core crc_itu_t serio_raw drm ata_piix > libata crc32c_intel i2c_core ixgbe(OE) vxlan e1000e ip6_udp_tunnel > udp_tunnel aacraid dca ptp pps_co > re > [545444.674783] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G OE > 3.10.0-327.10.1.el7.dsip.x86_64 #1 > [545444.675032] Hardware name: empty empty/S7010, BIOS 'V2.06 ' 03/31/2010 > [545444.675162] task: 880139c55c00 ti: 880139c84000 task.ti: > 880139c84000 > [545444.675400] RIP: 0010:[] [] > kmem_cache_alloc+0x75/0x1d0 > [545444.675641] RSP: 0018:88023fc23ce8 EFLAGS: 00010286 > [545444.675766] RAX: RBX: 8802302eab00 RCX: > 00010eb8edbe > [545444.676002] RDX: 00010eb8edbd RSI: 0020 RDI: > 88013b803700 > [545444.676237] RBP: 88023fc23d18 R08: 000175a0 R09: > 81517e70 > [545444.676472] R10: 006b R11: R12: > 88a005040200 > [545444.676706] R13: 0020 R14: 88013b803700 R15: > 88013b803700 > [545444.676942] FS: () GS:88023fc2() > knlGS: > [545444.677180] CS: 0010 DS: ES: CR0: 8005003b > [545444.677307] CR2: 88a005040200 CR3: 000237e63000 CR4: > 07e0 > [545444.677543] DR0: DR1: DR2: > > [545444.69] DR3: DR6: 0ff0 DR7: > 0400 > [545444.678014] Stack: > [545444.678127] 880237ea2040 8802302eab00 0280 > 0280 > [545444.678370] 0006 880236bb1b60 88023fc23d40 > 81517e70 > [545444.678614] 0280 8802302eab00 > 88023fc23d60 > [545444.678857] Call Trace: > [545444.678973] > > [545444.678982] > [545444.679100] [] build_skb+0x30/0x1d0 > [545444.679222] [] __alloc_rx_skb+0x63/0xb0 > [545444.679349] [] __netdev_alloc_skb+0x1b/0x40 > [545444.679492] [] ixgbe_clean_rx_irq+0xee/0xa50 [ixgbe] > [545444.679624] [] ? __napi_complete+0x1f/0x30 > [545444.679756] [] ixgbe_poll+0x2d8/0x6d0 [ixgbe] > [545444.679886] [] net_rx_action+0x152/0x240 > [545444.680015] [] __do_softirq+0xef/0x280 > [545444.680144] [] call_softirq+0x1c/0x30 > [545444.680277] [] do_softirq+0x65/0xa0 > [545444.680402] [] irq_exit+0x115/0x120 > [545444.680529] [] do_IRQ+0x58/0xf0 > [545444.680660] [] common_interrupt+0x6d/0x6d > [545444.680786] > [545444.680794] > [545444.680914] [] ? native_safe_halt+0x6/0x10 > [545444.681041] [] default_idle+0x1f/0xc0 > [545444.681168] [] arch_cpu_idle+0x26/0x30 > [545444.681297] [] cpu_startup_entry+0x245/0x290 > [545444.681427] [] start_secondary+0x1ba/0x230 > [545444.681554] Code: ce 00 00 49 8b 50 08 4d 8b 20 49 8b 40 10 4d 85 > e4 0f 84 1f 01 00 00 48 85 c0 0f 84 16 01 00 00 49 63 46 20 48 8d 4a > 01 4d 8b 06 <49> 8b 1c 04 4c > 89 e0 65 49 0f c7 08 0f 94 c0 84 c0 74 b9 49 63 > [545444.682056] RIP [] kmem_cache_alloc+0x75/0x1d0 > [545444.682186] RSP > [545444.682305] CR2: 88a005040200 > > > Every time description and call stack are the same. > > What can be cause of these crashes? > > Thanks. > I am wondering if your kernel has this commit id, 32b3e08fff60494cd1d281a39b51583edfd2b18f. As this seems to be added to fix issues that look very similar to the trace you are receiving. Nick
[PATCH] bridge:Fix incorrect variable assignment on error path in br_sysfs_addbr
This fixes the incorrect variable assignment on error path in
br_sysfs_addbr for when the call to kobject_create_and_add
fails to assign the value of -EINVAL to the returned variable of
err rather then incorrectly return zero making callers think this
function has succeededed due to the previous assignment being
assigned zero when assigning it the successful return value of
the call to sysfs_create_group which is zero.
Signed-off-by: Bastien Philbert
---
net/bridge/br_sysfs_br.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 6b80914..f4d40ed 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -870,6 +870,7 @@ int br_sysfs_addbr(struct net_device *dev)
br->ifobj = kobject_create_and_add(SYSFS_BRIDGE_PORT_SUBDIR, brobj);
if (!br->ifobj) {
+ err = -EINVAL;
pr_info("%s: can't add kobject (directory) %s/%s\n",
__func__, dev->name, SYSFS_BRIDGE_PORT_SUBDIR);
goto out3;
--
2.5.0
