* Re: [patch v1 1/2] Allow Mellanox network vendor to be configured if only I2C bus is configured
From: Leon Romanovsky @ 2017-08-12 14:37 UTC (permalink / raw)
To: Ohad Oz; +Cc: davem, netdev, jiri, saeedm, vadimp, system-sw-low-level
In-Reply-To: <1502385112-103765-1-git-send-email-ohado@mellanox.com>
[-- Attachment #1: Type: text/plain, Size: 868 bytes --]
On Thu, Aug 10, 2017 at 05:11:51PM +0000, Ohad Oz wrote:
> Patch allows Mellanox devices on system with no PCI, but with I2C only.
>
Did you test mlx5 device on such system? Did it work for you?
What is the changelog between v0 and v1 of these patches?
> Signed-off-by: Ohad Oz <ohado@mellanox.com>
> ---
> drivers/net/ethernet/mellanox/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/Kconfig b/drivers/net/ethernet/mellanox/Kconfig
> index 84a2007..0949741 100644
> --- a/drivers/net/ethernet/mellanox/Kconfig
> +++ b/drivers/net/ethernet/mellanox/Kconfig
> @@ -5,7 +5,7 @@
> config NET_VENDOR_MELLANOX
> bool "Mellanox devices"
> default y
> - depends on PCI
> + depends on PCI || I2C
> ---help---
> If you have a network (Ethernet) card belonging to this class, say Y.
>
> --
> 2.8.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] drivers: add vpls support
From: Amine Kherbouche @ 2017-08-12 13:40 UTC (permalink / raw)
To: Roopa Prabhu, David Lamparter; +Cc: netdev@vger.kernel.org
In-Reply-To: <CAJieiUjiDuhOeGZH9hMf4zTSKfkQ9bhzb9_XE9+5V1ZaDa_f2A@mail.gmail.com>
On 11/08/2017 17:14, Roopa Prabhu wrote:
> On Fri, Aug 11, 2017 at 5:55 AM, David Lamparter <equinox@diac24.net> wrote:
>> On Thu, Aug 10, 2017 at 10:28:37PM +0200, Amine Kherbouche wrote:
>>> This commit introduces the support of VPLS virtual device, that allows
>>> performing L2VPN multipoint to multipoint communication over MPLS PSN.
>>>
>>> VPLS device encap received ethernet frame over mpls packet and send it the
>>> output device, in the other direction, when receiving the right configured
>>> mpls packet, the matched mpls route calls the handler vpls function,
>>> then pulls out the mpls header and send it back the entry point via
>>> netif_rx().
>>>
>>> Two functions, mpls_entry_encode() and mpls_output_possible() are
>>> exported from mpls/internal.h to be able to use them inside vpls driver.
>>>
>>> Signed-off-by: Amine Kherbouche <amine.kherbouche@6wind.com>
> [snip]
>
>> [...]
>>> +union vpls_nh {
>>> + struct in6_addr addr6;
>>> + struct in_addr addr;
>>> +};
>>> +
>>> +struct vpls_dst {
>>> + struct net_device *dev;
>>> + union vpls_nh addr;
>>> + u32 label_in, label_out;
>>> + u32 id;
>>> + u16 vlan_id;
>> I looked at VLAN support and decided against it because the bridge layer
>> can handle this perfectly fine by using the bridge's vlan support to tag
>> a port's pvid.
> yes, agreed. there is no need for vlan here. The bridge can be
> configured with the required vlan
> mapping on the vpls port.
what if the output device cannot handle vlan encapsulation? because on my
example of configuration in the cover letter, I sent the vpls packets over
a simple physical net device (not a bridge nor a vlan port).
>
>
>>> + u8 via_table;
>>> + u8 flags;
>>> + u8 ttl;
>>> +};
>> [...]
>>> +struct vpls_priv {
>>> + struct net *encap_net;
>>> + struct vpls_dst dst;
>>> +};
>>> +
>>> +static struct nla_policy vpls_policy[IFLA_VPLS_MAX + 1] = {
>>> + [IFLA_VPLS_ID] = { .type = NLA_U32 },
>>> + [IFLA_VPLS_IN_LABEL] = { .type = NLA_U32 },
>>> + [IFLA_VPLS_OUT_LABEL] = { .type = NLA_U32 },
>>> + [IFLA_VPLS_OIF] = { .type = NLA_U32 },
>>> + [IFLA_VPLS_TTL] = { .type = NLA_U8 },
>>> + [IFLA_VPLS_VLANID] = { .type = NLA_U8 },
>>> + [IFLA_VPLS_NH] = { .type = NLA_U32 },
>>> + [IFLA_VPLS_NH6] = { .len = sizeof(struct in6_addr) },
>>> +};
>> The original patchset was point-to-multipoint in a single netdev, and
>> had some starts on optimized multicast support (which, admittedly, is a
>> bit of a fringe thing, but still.)
>>
> I had been thinking about this as a single netdevice as well...which
> can work with
> the bridge driver using per vlan dst_metadata infra (similar to vxlan
> single device and per vlan - vxlan mapping).
>
> Multiple netdevice with one per vlan-vpls-id will work as well... but
> starting with a single netdev
> will be better (helps with scaling).
That's why I added the vpls id, to have in the future many vpls tunnels with
a single device, so the vpls id let us keep tracking the device working like
the vni of vxlan. (vpls lwtunnels in the TODO list).
^ permalink raw reply
* Re: [PATCH 1/2] mpls: add handlers
From: Amine Kherbouche @ 2017-08-12 13:35 UTC (permalink / raw)
To: Roopa Prabhu, David Lamparter; +Cc: netdev@vger.kernel.org
In-Reply-To: <CAJieiUiDN0pk2GDb2+jBQzbjdPTpckSm23SCFGZ9mRHWfQ1H4w@mail.gmail.com>
On 11/08/2017 16:37, Roopa Prabhu wrote:
> On Fri, Aug 11, 2017 at 5:34 AM, David Lamparter <equinox@diac24.net> wrote:
>> On Thu, Aug 10, 2017 at 10:28:36PM +0200, Amine Kherbouche wrote:
>>> Mpls handler allows creation/deletion of mpls routes without using
>>> rtnetlink. When an incoming mpls packet matches this route, the saved
>>> function handler is called.
>> Since I originally authored this patch, I have come to believe that it
>> might be unneccessarily complicated. It is unlikely that a lot of
>> different "handlers" will exist here; the only things I can think of
>> are VPLS support and BIER-MPLS multicast replication. I'm not saying
>> it's a bad idea, but, well, this was in the README that I gave to 6WIND
>> with this code:
>>
>> ...
> yes, I would also prefer just exporting the functions and calling
> them directly instead of adding a
> handler layer. We can move to that later if it becomes necessary.
I understand that the handler layer is an overhead (as said by David's
note), and I agree with the solution for exporting the mpls functions that
allows route creation/deletion, but how about forwarding the right mpls
packet to the right vpls device with the device ptr? I don't see
another way.
^ permalink raw reply
* Re: Kernel 4.13.0-rc4-next-20170811 - IP Routing / Forwarding performance vs Core/RSS number / HT on
From: Jesper Dangaard Brouer @ 2017-08-12 12:23 UTC (permalink / raw)
To: Paweł Staszewski; +Cc: brouer, Linux Kernel Network Developers
In-Reply-To: <3ac1a817-5c62-2490-64e7-2512f0ee3b3e@itcare.pl>
On Fri, 11 Aug 2017 19:51:10 +0200 Paweł Staszewski <pstaszewski@itcare.pl> wrote:
> Hi
>
> I made some tests for performance comparison.
Thanks for doing this. Feel free to Cc me, if you do more of these
tests (so I don't miss them on the mailing list).
I don't understand stand if you are reporting a potential problem?
It would be good if you can provide a short summary section (of the
issue) in the _start_ of the email, and then provide all this nice data
afterwards, to back your case.
My understanding is, you report:
1. VLANs on ixgbe show a 30-40% slowdown
2. System stopped scaling after 7+ CPUs
> Tested HW (FORWARDING HOST):
>
> Intel(R) Xeon(R) Gold 6132 CPU @ 2.60GHz
Interesting, I've not heard about a Intel CPU called "Gold" before now,
but it does exist:
https://ark.intel.com/products/123541/Intel-Xeon-Gold-6132-Processor-19_25M-Cache-2_60-GHz
> Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection (rev 01)
This is one of my all time favorite NICs!
> Test diagram:
>
>
> TRAFFIC GENERATOR (ethX) -> (enp216s0f0 - RX Traffic) FORWARDING HOST
> (enp216s0f1(vlan1000) - TX Traffic) -> (ethY) SINK
>
> Forwarder traffic: UDP random ports from 9 to 19 with random hosts from
> 172.16.0.1 to 172.16.0.255
>
> TRAFFIC GENERATOR TX is stable 9.9Mpps (in kernel pktgen)
What kind of traffic flow? E.g. distribution, many/few source IPs...
> Settings used for FORWARDING HOST (changed param. was only number of RSS
> combined queues + set affinity assignment for them to fit with first
> numa node where 2x10G port card is installed)
>
> ixgbe driver used from kernel (in-kernel build - not a module)
>
Nice with a script showing you setup, thanks. I would be good if it had
comments, telling why you think this is a needed setup adjustment.
> #!/bin/sh
> ifc='enp216s0f0 enp216s0f1'
> for i in $ifc
> do
> ip link set up dev $i
> ethtool -A $i autoneg off rx off tx off
Good:
Turning off Ethernet flow control, to avoid receiver being the
bottleneck via pause-frames.
> ethtool -G $i rx 4096 tx 1024
You adjust the RX and TX ring queue sizes, this have effects that you
don't realize. Especially for the ixgbe driver, which have a page
recycle trick tied to the RX ring queue size.
> ip link set $i txqueuelen 1000
Setting tx queue len to the default 1000 seems redundant.
> ethtool -C $i rx-usecs 10
Adjusting this also have effects you might not realize. This actually
also affect the page recycle scheme of ixgbe. And can sometimes be
used to solve stalling on DMA TX completions, which could be you issue
here.
> ethtool -L $i combined 16
> ethtool -K $i gro on tso on gso off sg on l2-fwd-offload off
> tx-nocache-copy on ntuple on
Here are many setting above.
GRO/GSO/TSO for _forwarding_ is actually bad... in my tests, enabling
this result in approx 10% slowdown.
AFAIK "tx-nocache-copy on" was also determined to be a bad option.
The "ntuple on" AFAIK disables the flow-director in the NIC. I though
this would actually help VLAN traffic, but I guess not.
> ethtool -N $i rx-flow-hash udp4 sdfn
Why do you change the NICs flow-hash?
> done
>
> ip link set up dev enp216s0f0
> ip link set up dev enp216s0f1
>
> ip a a 10.0.0.1/30 dev enp216s0f0
>
> ip link add link enp216s0f1 name vlan1000 type vlan id 1000
> ip link set up dev vlan1000
> ip a a 10.0.0.5/30 dev vlan1000
>
>
> ip route add 172.16.0.0/12 via 10.0.0.6
>
> ./set_irq_affinity.sh -x 14-27,42-43 enp216s0f0
> ./set_irq_affinity.sh -x 14-27,42-43 enp216s0f1
> #cat /sys/devices/system/node/node1/cpulist
> #14-27,42-55
> #cat /sys/devices/system/node/node0/cpulist
> #0-13,28-41
Is this a NUMA system?
> #################################################
>
>
> Looks like forwarding performance when using vlans on ixgbe is less that
> without vlans for about 30-40% (wondering if this is some vlan
> offloading problem and ixgbe)
I would see this as a problem/bug that enabling VLANs cost this much.
> settings below:
>
> ethtool -k enp216s0f0
> Features for enp216s0f0:
> Cannot get device udp-fragmentation-offload settings: Operation not
> supported
> rx-checksumming: on
> tx-checksumming: on
> tx-checksum-ipv4: off [fixed]
> tx-checksum-ip-generic: on
> tx-checksum-ipv6: off [fixed]
> tx-checksum-fcoe-crc: off [fixed]
> tx-checksum-sctp: on
> scatter-gather: on
> tx-scatter-gather: on
> tx-scatter-gather-fraglist: off [fixed]
> tcp-segmentation-offload: on
> tx-tcp-segmentation: on
> tx-tcp-ecn-segmentation: off [fixed]
> tx-tcp-mangleid-segmentation: on
> tx-tcp6-segmentation: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: off
> generic-receive-offload: on
> large-receive-offload: off
> rx-vlan-offload: on
> tx-vlan-offload: on
> ntuple-filters: on
> receive-hashing: on
> highdma: on [fixed]
> rx-vlan-filter: on
> vlan-challenged: off [fixed]
> tx-lockless: off [fixed]
> netns-local: off [fixed]
> tx-gso-robust: off [fixed]
> tx-fcoe-segmentation: off [fixed]
> tx-gre-segmentation: on
> tx-gre-csum-segmentation: on
> tx-ipxip4-segmentation: on
> tx-ipxip6-segmentation: on
> tx-udp_tnl-segmentation: on
> tx-udp_tnl-csum-segmentation: on
> tx-gso-partial: on
> tx-sctp-segmentation: off [fixed]
> tx-esp-segmentation: off [fixed]
> fcoe-mtu: off [fixed]
> tx-nocache-copy: on
> loopback: off [fixed]
> rx-fcs: off [fixed]
> rx-all: off
> tx-vlan-stag-hw-insert: off [fixed]
> rx-vlan-stag-hw-parse: off [fixed]
> rx-vlan-stag-filter: off [fixed]
> l2-fwd-offload: off
> hw-tc-offload: off
> esp-hw-offload: off [fixed]
> esp-tx-csum-hw-offload: off [fixed]
> rx-udp_tunnel-port-offload: on
>
>
> Another thing is that forwarding performance does not scale with number
> of cores when 7+ cores are reached
I've seen problems with using Hyper-Threading CPUs. Could it be that
above 7 CPUs you are starting to use sibling-cores ?
> perf top:
>
> PerfTop: 77835 irqs/sec kernel:99.7% exact: 0.0% [4000Hz
> cycles], (all, 56 CPUs)
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> 16.32% [kernel] [k] skb_dst_force
> 16.30% [kernel] [k] dst_release
> 15.11% [kernel] [k] rt_cache_valid
> 12.62% [kernel] [k] ipv4_mtu
It seems a little strange that these 4 functions are on the top
> 5.60% [kernel] [k] do_raw_spin_lock
Why is calling/taking this lock? (Use perf call-graph recording).
> 3.03% [kernel] [k] fib_table_lookup
> 2.70% [kernel] [k] ip_finish_output2
> 2.10% [kernel] [k] dev_gro_receive
> 1.89% [kernel] [k] eth_type_trans
> 1.81% [kernel] [k] ixgbe_poll
> 1.15% [kernel] [k] ixgbe_xmit_frame_ring
> 1.06% [kernel] [k] __build_skb
> 1.04% [kernel] [k] __dev_queue_xmit
> 0.97% [kernel] [k] ip_rcv
> 0.78% [kernel] [k] netif_skb_features
> 0.74% [kernel] [k] ipt_do_table
Unloading netfilter modules, will give more performance, but it
semifake to do so.
> 0.70% [kernel] [k] acpi_processor_ffh_cstate_enter
> 0.64% [kernel] [k] ip_forward
> 0.59% [kernel] [k] __netif_receive_skb_core
> 0.55% [kernel] [k] dev_hard_start_xmit
> 0.53% [kernel] [k] ip_route_input_rcu
> 0.53% [kernel] [k] ip_rcv_finish
> 0.51% [kernel] [k] page_frag_free
> 0.50% [kernel] [k] kmem_cache_alloc
> 0.50% [kernel] [k] udp_v4_early_demux
> 0.44% [kernel] [k] skb_release_data
> 0.42% [kernel] [k] inet_gro_receive
> 0.40% [kernel] [k] sch_direct_xmit
> 0.39% [kernel] [k] __local_bh_enable_ip
> 0.33% [kernel] [k] netdev_pick_tx
> 0.33% [kernel] [k] validate_xmit_skb
> 0.28% [kernel] [k] fib_validate_source
> 0.27% [kernel] [k] deliver_ptype_list_skb
> 0.25% [kernel] [k] eth_header
> 0.23% [kernel] [k] get_dma_ops
> 0.22% [kernel] [k] skb_network_protocol
> 0.21% [kernel] [k] ip_output
> 0.21% [kernel] [k] vlan_dev_hard_start_xmit
> 0.20% [kernel] [k] ixgbe_alloc_rx_buffers
> 0.18% [kernel] [k] nf_hook_slow
> 0.18% [kernel] [k] apic_timer_interrupt
> 0.18% [kernel] [k] virt_to_head_page
> 0.18% [kernel] [k] build_skb
> 0.16% [kernel] [k] swiotlb_map_page
> 0.16% [kernel] [k] ip_finish_output
> 0.16% [kernel] [k] udp4_gro_receive
>
>
> RESULTS:
>
> CSV format - delimeter ";"
>
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;1;64;1470912;88247040;1470720;85305530
> 1;1;64;1470912;88285440;1470977;85335110
> 2;1;64;1470464;88247040;1470402;85290508
> 3;1;64;1471424;88262400;1471230;85353728
> 4;1;64;1468736;88166400;1468672;85201652
> 5;1;64;1470016;88181760;1469949;85234944
> 6;1;64;1470720;88247040;1470466;85290624
> 7;1;64;1471232;88277760;1471167;85346246
> 8;1;64;1469184;88170240;1469249;85216326
> 9;1;64;1470592;88227840;1470847;85294394
Single core 1.47Mpps seems a little low, I would expect 2Mpps.
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;2;64;2413120;144802560;2413245;139975924
> 1;2;64;2415296;144913920;2415356;140098188
> 2;2;64;2416768;144898560;2416573;140105670
> 3;2;64;2418176;145056000;2418110;140261806
> 4;2;64;2416512;144990720;2416509;140172950
> 5;2;64;2415168;144860160;2414466;140064780
> 6;2;64;2416960;144983040;2416833;140190930
> 7;2;64;2413632;144768000;2413568;140001734
> 8;2;64;2415296;144898560;2414589;140087168
> 9;2;64;2416576;144963840;2416892;140190930
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;3;64;3419008;205155840;3418882;198239244
> 1;3;64;3428032;205585920;3427971;198744234
> 2;3;64;3425472;205536000;3425344;198677260
> 3;3;64;3425088;205470720;3425156;198603136
> 4;3;64;3427648;205693440;3426883;198773888
> 5;3;64;3426880;205670400;3427392;198796044
> 6;3;64;3429120;205678080;3430140;198848186
> 7;3;64;3422976;205355520;3423490;198458136
> 8;3;64;3423168;205336320;3423486;198495372
> 9;3;64;3424384;205493760;3425538;198617868
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;4;64;4406464;264364800;4405244;255560296
> 1;4;64;4404672;264349440;4405122;255541504
> 2;4;64;4402368;264049920;4403326;255188864
> 3;4;64;4401344;264076800;4400702;255207134
> 4;4;64;4385536;263074560;4386620;254312716
> 5;4;64;4386560;263189760;4385404;254379532
> 6;4;64;4398784;263857920;4399031;255025288
> 7;4;64;4407232;264445440;4407998;255637900
> 8;4;64;4413184;264698880;4413758;255875816
> 9;4;64;4411328;264526080;4411906;255712372
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;5;64;5094464;305871360;5094464;295657262
> 1;5;64;5090816;305514240;5091201;295274810
> 2;5;64;5088384;305387520;5089792;295175108
> 3;5;64;5079296;304869120;5079484;294680368
> 4;5;64;5092992;305544960;5094207;295349166
> 5;5;64;5092416;305502720;5093372;295334260
> 6;5;64;5080896;304896000;5081090;294677004
> 7;5;64;5085376;305114880;5086401;294933058
> 8;5;64;5092544;305575680;5092036;295356938
> 9;5;64;5093056;305652480;5093832;295449506
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;6;64;5705088;342351360;5705784;330965110
> 1;6;64;5710272;342743040;5707591;331373952
> 2;6;64;5703424;342182400;5701826;330776552
> 3;6;64;5708736;342604800;5707963;331147462
> 4;6;64;5710144;342654720;5712067;331202910
> 5;6;64;5712064;342777600;5711361;331292288
> 6;6;64;5710144;342585600;5708607;331144272
> 7;6;64;5699840;342021120;5697853;330609222
> 8;6;64;5701184;342124800;5702909;330653592
> 9;6;64;5711360;342735360;5713283;331247686
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;7;64;6244416;374603520;6243591;362180072
> 1;7;64;6230912;374016000;6231490;361534126
> 2;7;64;6244800;374776320;6244866;362224326
> 3;7;64;6238720;374376960;6238261;361838510
> 4;7;64;6218816;373079040;6220413;360683962
> 5;7;64;6224320;373566720;6225086;361017404
> 6;7;64;6224000;373570560;6221370;360936088
> 7;7;64;6210048;372741120;6210627;360212654
> 8;7;64;6231616;374035200;6231537;361445502
> 9;7;64;6227840;373724160;6228802;361162752
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;8;64;6251840;375144960;6251849;362609678
> 1;8;64;6250816;375014400;6250881;362547038
> 2;8;64;6257728;375432960;6257160;362911104
> 3;8;64;6255552;375325440;6255622;362822074
> 4;8;64;6243776;374576640;6243270;362120622
> 5;8;64;6237184;374296320;6237690;361790080
> 6;8;64;6240960;374415360;6240714;361927366
> 7;8;64;6222784;373317120;6223746;360854424
> 8;8;64;6225920;373593600;6227014;361154980
> 9;8;64;6238528;374304000;6237701;361845238
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;14;64;6486144;389184000;6486135;376236488
> 1;14;64;6454912;387390720;6454222;374466734
> 2;14;64;6441152;386480640;6440431;373572780
> 3;14;64;6450240;386972160;6450870;374070014
> 4;14;64;6465600;387997440;6467221;375089654
> 5;14;64;6448384;386860800;6448000;373980230
> 6;14;64;6452352;387095040;6452148;374168904
> 7;14;64;6441984;386507520;6443203;373665058
> 8;14;64;6456704;387340800;6455744;374429092
> 9;14;64;6464640;387901440;6465218;374949004
> ID;CPU_CORES / RSS QUEUES;PKT_SIZE;PPS_RX;BPS_RX;PPS_TX;BPS_TX
> 0;16;64;6939008;416325120;6938696;402411192
> 1;16;64;6941952;416444160;6941745;402558918
> 2;16;64;6960576;417584640;6960707;403698718
> 3;16;64;6940736;416486400;6941820;402503876
> 4;16;64;6927680;415741440;6927420;401853870
> 5;16;64;6929792;415687680;6929917;401839196
> 6;16;64;6950400;416989440;6950661;403026166
> 7;16;64;6953664;417216000;6953454;403260544
> 8;16;64;6948480;416851200;6948800;403023266
> 9;16;64;6924160;415422720;6924092;401542468
I've seen Linux scale beyond 6.9Mpps, thus I also see this as an
issue/bug. You could be stalling on DMA TX completion being too slow,
but you already increased the interval and increased the TX ring queue
size. You could play with those setting and see if it changes this?
Could you try my napi_monitor tool in:
https://github.com/netoptimizer/prototype-kernel/tree/master/kernel/samples/bpf
Also provide the output from:
mpstat -P ALL -u -I SCPU -I SUM 2
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [iproute PATCH 24/51] ifstat, nstat: Check fdopen() return value
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Prevent passing NULL FILE pointer to fgets() later.
Fix both tools in a single patch since the code changes are basically
identical.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
misc/ifstat.c | 16 +++++++++++-----
misc/nstat.c | 16 +++++++++++-----
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/misc/ifstat.c b/misc/ifstat.c
index 8fa354265a9a1..6c8cfa0bd94f5 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -988,12 +988,18 @@ int main(int argc, char *argv[])
&& verify_forging(fd) == 0) {
FILE *sfp = fdopen(fd, "r");
- load_raw_table(sfp);
- if (hist_db && source_mismatch) {
- fprintf(stderr, "ifstat: history is stale, ignoring it.\n");
- hist_db = NULL;
+ if (!sfp) {
+ fprintf(stderr, "ifstat: fdopen failed: %s\n",
+ strerror(errno));
+ close(fd);
+ } else {
+ load_raw_table(sfp);
+ if (hist_db && source_mismatch) {
+ fprintf(stderr, "ifstat: history is stale, ignoring it.\n");
+ hist_db = NULL;
+ }
+ fclose(sfp);
}
- fclose(sfp);
} else {
if (fd >= 0)
close(fd);
diff --git a/misc/nstat.c b/misc/nstat.c
index 1212b1f2c8128..a4dd405d43a93 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -706,12 +706,18 @@ int main(int argc, char *argv[])
&& verify_forging(fd) == 0) {
FILE *sfp = fdopen(fd, "r");
- load_good_table(sfp);
- if (hist_db && source_mismatch) {
- fprintf(stderr, "nstat: history is stale, ignoring it.\n");
- hist_db = NULL;
+ if (!sfp) {
+ fprintf(stderr, "nstat: fdopen failed: %s\n",
+ strerror(errno));
+ close(fd);
+ } else {
+ load_good_table(sfp);
+ if (hist_db && source_mismatch) {
+ fprintf(stderr, "nstat: history is stale, ignoring it.\n");
+ hist_db = NULL;
+ }
+ fclose(sfp);
}
- fclose(sfp);
} else {
if (fd >= 0)
close(fd);
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 02/51] devlink: No need for this self-assignment
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
dl_argv_handle_both() will either assign to handle_bit or error out in
which case the variable is not used by the caller.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
devlink/devlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index de41a9f4aae10..9fb3c7d2a91e9 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -787,7 +787,7 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
int err;
if (o_required & DL_OPT_HANDLE && o_required & DL_OPT_HANDLEP) {
- uint32_t handle_bit = handle_bit;
+ uint32_t handle_bit;
err = dl_argv_handle_both(dl, &opts->bus_name, &opts->dev_name,
&opts->port_index, &handle_bit);
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 49/51] lib/ll_map: Make sure im->name is NULL-terminated
From: Phil Sutter @ 2017-08-12 12:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
lib/ll_map.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/ll_map.c b/lib/ll_map.c
index 4e4556c9ac80b..4d06eb69f138a 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c
@@ -120,11 +120,11 @@ int ll_remember_index(const struct sockaddr_nl *who,
return 0;
}
- im = malloc(sizeof(*im));
+ im = calloc(1, sizeof(*im));
if (im == NULL)
return 0;
im->index = ifi->ifi_index;
- strcpy(im->name, ifname);
+ strncpy(im->name, ifname, IFNAMSIZ - 1);
im->type = ifi->ifi_type;
im->flags = ifi->ifi_flags;
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 32/51] ss: Make sure index variable is >= 0
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
This shouldn't happen but relying upon external data without checking
may lead to unexpected results.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
misc/ss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/misc/ss.c b/misc/ss.c
index d767b1103ea81..4d2f75b571ea6 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2003,7 +2003,7 @@ static void tcp_timer_print(struct tcpstat *s)
"unknown"
};
- if (s->timer) {
+ if (s->timer >= 0) {
if (s->timer > 4)
s->timer = 5;
printf(" timer:(%s,%s,%d)",
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 44/51] tipc/bearer: Fix resource leak in error path
From: Phil Sutter @ 2017-08-12 12:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
tipc/bearer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tipc/bearer.c b/tipc/bearer.c
index 810344f672af1..c3d4491f8f6ef 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -163,6 +163,7 @@ static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
if (!remip) {
if (generate_multicast(loc->ai_family, buf, sizeof(buf))) {
fprintf(stderr, "Failed to generate multicast address\n");
+ freeaddrinfo(loc);
return -EINVAL;
}
remip = buf;
@@ -177,6 +178,8 @@ static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
if (rem->ai_family != loc->ai_family) {
fprintf(stderr, "UDP local and remote AF mismatch\n");
+ freeaddrinfo(rem);
+ freeaddrinfo(loc);
return -EINVAL;
}
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 46/51] tipc/node: Fix socket fd check in cmd_node_get_addr()
From: Phil Sutter @ 2017-08-12 12:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
socket() returns -1 on error, not 0.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
tipc/node.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tipc/node.c b/tipc/node.c
index 201fe1a4df3bd..fe085aec9b4ac 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -109,7 +109,8 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
socklen_t sz = sizeof(struct sockaddr_tipc);
struct sockaddr_tipc addr;
- if (!(sk = socket(AF_TIPC, SOCK_RDM, 0))) {
+ sk = socket(AF_TIPC, SOCK_RDM, 0);
+ if (sk < 0) {
fprintf(stderr, "opening TIPC socket: %s\n", strerror(errno));
return -1;
}
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 12/51] iproute_lwtunnel: csum_mode value checking was ineffective
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
ila_csum_name2mode() returning -1 on error but being declared as
returning __u8 doesn't make much sense. Change the code to correctly
detect this issue. Checking for __u8 overruns shouldn't be necessary
though since ila_csum_name2mode() return values are well-defined.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/iproute_lwtunnel.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 5c0c7d110d23e..398ab5e077ed8 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -171,7 +171,7 @@ static char *ila_csum_mode2name(__u8 csum_mode)
}
}
-static __u8 ila_csum_name2mode(char *name)
+static int ila_csum_name2mode(char *name)
{
if (strcmp(name, "adj-transport") == 0)
return ILA_CSUM_ADJUST_TRANSPORT;
@@ -517,7 +517,7 @@ static int parse_encap_ila(struct rtattr *rta, size_t len,
while (argc > 0) {
if (strcmp(*argv, "csum-mode") == 0) {
- __u8 csum_mode;
+ int csum_mode;
NEXT_ARG();
@@ -526,7 +526,8 @@ static int parse_encap_ila(struct rtattr *rta, size_t len,
invarg("\"csum-mode\" value is invalid\n",
*argv);
- rta_addattr8(rta, 1024, ILA_ATTR_CSUM_MODE, csum_mode);
+ rta_addattr8(rta, 1024, ILA_ATTR_CSUM_MODE,
+ (__u8)csum_mode);
argc--; argv++;
} else {
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 15/51] ipvrf: Fix error path of vrf_switch()
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Apart from trying to close(-1), this also leaked memory.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/ipvrf.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index 92e2db98ca7d7..75cc026d072b8 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -373,12 +373,12 @@ static int vrf_switch(const char *name)
/* -1 on length to add '/' to the end */
if (ipvrf_get_netns(netns, sizeof(netns) - 1) < 0)
- return -1;
+ goto out;
if (vrf_path(vpath, sizeof(vpath)) < 0) {
fprintf(stderr, "Failed to get base cgroup path: %s\n",
strerror(errno));
- return -1;
+ goto out;
}
/* if path already ends in netns then don't add it again */
@@ -429,13 +429,14 @@ static int vrf_switch(const char *name)
snprintf(pid, sizeof(pid), "%d", getpid());
if (write(fd, pid, strlen(pid)) < 0) {
fprintf(stderr, "Failed to join cgroup\n");
- goto out;
+ goto out2;
}
rc = 0;
+out2:
+ close(fd);
out:
free(mnt);
- close(fd);
return rc;
}
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 43/51] tc/tc_filter: Make sure filter name is not empty
From: Phil Sutter @ 2017-08-12 12:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
The later check for 'k[0] != 0' requires a non-empty filter name,
otherwise NULL pointer dereference in 'q' might happen.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
tc/tc_filter.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index b13fb9185d4fd..a799edb35886d 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
usage();
return 0;
} else {
+ if (!strlen(*argv))
+ invarg("invalid filter name", *argv);
+
strncpy(k, *argv, sizeof(k)-1);
q = get_filter_kind(k);
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 17/51] lib/bpf: Don't leak fp in bpf_find_mntpt()
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
If fopen() succeeded but len != PATH_MAX, the function leaks the open
FILE pointer. Fix this by checking len value before calling fopen().
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
lib/bpf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/bpf.c b/lib/bpf.c
index 4f52ad4a8f023..1dcb261dc915f 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -537,8 +537,11 @@ static const char *bpf_find_mntpt(const char *fstype, unsigned long magic,
}
}
+ if (len != PATH_MAX)
+ return NULL;
+
fp = fopen("/proc/mounts", "r");
- if (fp == NULL || len != PATH_MAX)
+ if (fp == NULL)
return NULL;
while (fscanf(fp, "%*s %" textify(PATH_MAX) "s %99s %*s %*d %*d\n",
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 07/51] ipmaddr: Avoid accessing uninitialized data
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Looks like this can only happen if /proc/net/igmp is malformed, but
better be sure.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/ipmaddr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 4f726fdd976f1..85a69e779563d 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c
@@ -136,7 +136,7 @@ static void read_igmp(struct ma_info **result_p)
while (fgets(buf, sizeof(buf), fp)) {
struct ma_info *ma;
- size_t len;
+ size_t len = 0;
if (buf[0] != '\t') {
sscanf(buf, "%d%s", &m.index, m.name);
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 37/51] netem/maketable: Check return value of fscanf()
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
netem/maketable.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/netem/maketable.c b/netem/maketable.c
index ad660e7d457f0..ccb8f0c68b062 100644
--- a/netem/maketable.c
+++ b/netem/maketable.c
@@ -38,8 +38,8 @@ readdoubles(FILE *fp, int *number)
}
for (i=0; i<limit; ++i){
- fscanf(fp, "%lf", &x[i]);
- if (feof(fp))
+ if (fscanf(fp, "%lf", &x[i]) != 1 ||
+ feof(fp))
break;
++n;
}
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 06/51] iplink_vrf: Complain if main table is not found
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/iplink_vrf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ip/iplink_vrf.c b/ip/iplink_vrf.c
index 917630e853375..809eda5de8f6e 100644
--- a/ip/iplink_vrf.c
+++ b/ip/iplink_vrf.c
@@ -131,7 +131,10 @@ __u32 ipvrf_get_table(const char *name)
&answer.n, sizeof(answer)) < 0) {
/* special case "default" vrf to be the main table */
if (errno == ENODEV && !strcmp(name, "default"))
- rtnl_rttable_a2n(&tb_id, "main");
+ if (rtnl_rttable_a2n(&tb_id, "main"))
+ fprintf(stderr,
+ "BUG: RTTable \"main\" not found.\n");
+
return tb_id;
}
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 42/51] tc/q_netem: Don't dereference possibly NULL pointer
From: Phil Sutter @ 2017-08-12 12:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Assuming 'opt' might be NULL, move the call to RTA_PAYLOAD to after the
check since it dereferences its parameter.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
tc/q_netem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tc/q_netem.c b/tc/q_netem.c
index 0975ae111de97..7e3330512041a 100644
--- a/tc/q_netem.c
+++ b/tc/q_netem.c
@@ -538,7 +538,7 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
int *ecn = NULL;
struct tc_netem_qopt qopt;
const struct tc_netem_rate *rate = NULL;
- int len = RTA_PAYLOAD(opt) - sizeof(qopt);
+ int len;
__u64 rate64 = 0;
SPRINT_BUF(b1);
@@ -546,6 +546,8 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
if (opt == NULL)
return 0;
+ len = RTA_PAYLOAD(opt) - sizeof(qopt);
+
if (len < 0) {
fprintf(stderr, "options size error\n");
return -1;
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 16/51] xfrm_state: Make sure alg_name is NULL-terminated
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/xfrm_state.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index e11c93bf1c3b5..7c0389038986e 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -125,7 +125,8 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
fprintf(stderr, "warning: ALGO-NAME/ALGO-KEYMAT values will be sent to the kernel promiscuously! (verifying them isn't implemented yet)\n");
#endif
- strncpy(alg->alg_name, name, sizeof(alg->alg_name));
+ strncpy(alg->alg_name, name, sizeof(alg->alg_name) - 1);
+ alg->alg_name[sizeof(alg->alg_name) - 1] = '\0';
if (slen > 2 && strncmp(key, "0x", 2) == 0) {
/* split two chars "0x" from the top */
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 50/51] Check user supplied interface name lengths
From: Phil Sutter @ 2017-08-12 12:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
The original problem was that something like:
| strncpy(ifr.ifr_name, *argv, IFNAMSIZ);
might leave ifr.ifr_name unterminated if length of *argv exceeds
IFNAMSIZ. In order to fix this, I thought about replacing all those
cases with (equivalent) calls to snprintf() or even introducing
strlcpy(). But as Ulrich Drepper correctly pointed out when rejecting
the latter from being added to glibc, truncating a string without
notifying the user is not to be considered good practice. So let's
excercise what he suggested and reject empty or overlong interface names
right from the start - this way calls to strncpy() like shown above
become safe and the user has a chance to reconsider what he was trying
to do.
Note that this doesn't add calls to assert_valid_dev_name() to all
places where user supplied interface name is parsed. In many cases, the
interface must exist already and is therefore being looked up using
ll_name_to_index(), so if_nametoindex() will perform the necessary
checks already.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
include/utils.h | 1 +
ip/ip6tunnel.c | 6 ++++--
ip/ipl2tp.c | 1 +
ip/iplink.c | 27 ++++++++-------------------
ip/ipmaddr.c | 1 +
ip/iprule.c | 4 ++++
ip/iptunnel.c | 12 ++++++++----
ip/iptuntap.c | 4 +++-
lib/utils.c | 8 ++++++++
misc/arpd.c | 1 +
10 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/include/utils.h b/include/utils.h
index 6080b962fb411..103def33c92f3 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -132,6 +132,7 @@ void missarg(const char *) __attribute__((noreturn));
void invarg(const char *, const char *) __attribute__((noreturn));
void duparg(const char *, const char *) __attribute__((noreturn));
void duparg2(const char *, const char *) __attribute__((noreturn));
+void assert_valid_dev_name(const char *, const char *);
int matches(const char *arg, const char *pattern);
int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index b4a7def144226..b3ccfcf87be8f 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -180,7 +180,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
memcpy(&p->laddr, &laddr.data, sizeof(p->laddr));
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ - 1);
+ assert_valid_dev_name("dev", *argv);
+ strncpy(medium, *argv, IFNAMSIZ);
} else if (strcmp(*argv, "encaplimit") == 0) {
NEXT_ARG();
if (strcmp(*argv, "none") == 0) {
@@ -273,7 +274,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
usage();
if (p->name[0])
duparg2("name", *argv);
- strncpy(p->name, *argv, IFNAMSIZ - 1);
+ assert_valid_dev_name("name", *argv);
+ strncpy(p->name, *argv, IFNAMSIZ);
if (cmd == SIOCCHGTUNNEL && count == 0) {
struct ip6_tnl_parm2 old_p = {};
diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
index 88664c909e11f..cdf9f4d8425d4 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -545,6 +545,7 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
}
} else if (strcmp(*argv, "name") == 0) {
NEXT_ARG();
+ assert_valid_dev_name("name", *argv);
p->ifname = *argv;
} else if (strcmp(*argv, "remote") == 0) {
NEXT_ARG();
diff --git a/ip/iplink.c b/ip/iplink.c
index 5aff2fde38dae..8f76f0467fc9c 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -869,7 +869,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
{
- int len;
char *dev = NULL;
char *name = NULL;
char *link = NULL;
@@ -959,13 +958,9 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
}
if (name) {
- len = strlen(name) + 1;
- if (len == 1)
- invarg("\"\" is not a valid device identifier\n",
- "name");
- if (len > IFNAMSIZ)
- invarg("\"name\" too long\n", name);
- addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
+ assert_valid_dev_name("name", name);
+ addattr_l(&req.n, sizeof(req),
+ IFLA_IFNAME, name, strlen(name) + 1);
}
if (type) {
@@ -1015,7 +1010,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
{
- int len;
struct iplink_req req = {
.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
.n.nlmsg_flags = NLM_F_REQUEST | flags,
@@ -1028,13 +1022,9 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
} answer;
if (name) {
- len = strlen(name) + 1;
- if (len == 1)
- invarg("\"\" is not a valid device identifier\n",
- "name");
- if (len > IFNAMSIZ)
- invarg("\"name\" too long\n", name);
- addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
+ assert_valid_dev_name("name", name);
+ addattr_l(&req.n, sizeof(req),
+ IFLA_IFNAME, name, strlen(name) + 1);
}
addattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);
@@ -1357,6 +1347,7 @@ static int do_set(int argc, char **argv)
"Not enough of information: \"dev\" argument is required.\n");
exit(-1);
}
+ assert_valid_dev_name("dev", dev);
if (newaddr || newbrd) {
halen = get_address(dev, &htype);
@@ -1375,9 +1366,7 @@ static int do_set(int argc, char **argv)
}
if (newname && strcmp(dev, newname)) {
- if (strlen(newname) == 0)
- invarg("\"\" is not a valid device identifier\n",
- "name");
+ assert_valid_dev_name("name", newname);
if (do_changename(dev, newname) < 0)
return -1;
dev = newname;
diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 85a69e779563d..753e32d5807c0 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c
@@ -284,6 +284,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
NEXT_ARG();
if (ifr.ifr_name[0])
duparg("dev", *argv);
+ assert_valid_dev_name("dev", *argv);
strncpy(ifr.ifr_name, *argv, IFNAMSIZ);
} else {
if (matches(*argv, "address") == 0) {
diff --git a/ip/iprule.c b/ip/iprule.c
index 8313138db815f..0184e922053ae 100644
--- a/ip/iprule.c
+++ b/ip/iprule.c
@@ -472,10 +472,12 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)
} else if (strcmp(*argv, "dev") == 0 ||
strcmp(*argv, "iif") == 0) {
NEXT_ARG();
+ assert_valid_dev_name("iif", *argv);
strncpy(filter.iif, *argv, IFNAMSIZ);
filter.iifmask = 1;
} else if (strcmp(*argv, "oif") == 0) {
NEXT_ARG();
+ assert_valid_dev_name("oif", *argv);
strncpy(filter.oif, *argv, IFNAMSIZ);
filter.oifmask = 1;
} else if (strcmp(*argv, "l3mdev") == 0) {
@@ -695,10 +697,12 @@ static int iprule_modify(int cmd, int argc, char **argv)
} else if (strcmp(*argv, "dev") == 0 ||
strcmp(*argv, "iif") == 0) {
NEXT_ARG();
+ assert_valid_dev_name("iif", *argv);
addattr_l(&req.n, sizeof(req), FRA_IFNAME,
*argv, strlen(*argv)+1);
} else if (strcmp(*argv, "oif") == 0) {
NEXT_ARG();
+ assert_valid_dev_name("oif", *argv);
addattr_l(&req.n, sizeof(req), FRA_OIFNAME,
*argv, strlen(*argv)+1);
} else if (strcmp(*argv, "l3mdev") == 0) {
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 105d0f5576f1a..b9dd19c6f83c5 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -139,7 +139,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
p->iph.saddr = htonl(INADDR_ANY);
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ - 1);
+ assert_valid_dev_name("dev", *argv);
+ strncpy(medium, *argv, IFNAMSIZ);
} else if (strcmp(*argv, "ttl") == 0 ||
strcmp(*argv, "hoplimit") == 0 ||
strcmp(*argv, "hlim") == 0) {
@@ -178,7 +179,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
if (p->name[0])
duparg2("name", *argv);
- strncpy(p->name, *argv, IFNAMSIZ - 1);
+ assert_valid_dev_name("name", *argv);
+ strncpy(p->name, *argv, IFNAMSIZ);
if (cmd == SIOCCHGTUNNEL && count == 0) {
struct ip_tunnel_parm old_p = {};
@@ -488,7 +490,8 @@ static int do_prl(int argc, char **argv)
count++;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ-1);
+ assert_valid_dev_name("dev", *argv);
+ strncpy(medium, *argv, IFNAMSIZ);
devname++;
} else {
fprintf(stderr,
@@ -537,7 +540,8 @@ static int do_6rd(int argc, char **argv)
cmd = SIOCDEL6RD;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ-1);
+ assert_valid_dev_name("dev", *argv);
+ strncpy(medium, *argv, IFNAMSIZ);
devname++;
} else {
fprintf(stderr,
diff --git a/ip/iptuntap.c b/ip/iptuntap.c
index 451f7f0eac6bb..062f3d6ed036e 100644
--- a/ip/iptuntap.c
+++ b/ip/iptuntap.c
@@ -176,7 +176,8 @@ static int parse_args(int argc, char **argv,
ifr->ifr_flags |= IFF_MULTI_QUEUE;
} else if (matches(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(ifr->ifr_name, *argv, IFNAMSIZ-1);
+ assert_valid_dev_name("dev", *argv);
+ strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
} else {
if (matches(*argv, "name") == 0) {
NEXT_ARG();
@@ -184,6 +185,7 @@ static int parse_args(int argc, char **argv,
usage();
if (ifr->ifr_name[0])
duparg2("name", *argv);
+ assert_valid_dev_name("name", *argv);
strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
}
count++;
diff --git a/lib/utils.c b/lib/utils.c
index 9143ed2284870..002063075fd61 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -699,6 +699,14 @@ void duparg2(const char *key, const char *arg)
exit(-1);
}
+void assert_valid_dev_name(const char *arg, const char *name)
+{
+ size_t len = strlen(name);
+
+ if (len < 1 || len >= IFNAMSIZ)
+ invarg("empty or overlong interface name.", len ? name : arg);
+}
+
int matches(const char *cmd, const char *pattern)
{
int len = strlen(cmd);
diff --git a/misc/arpd.c b/misc/arpd.c
index bfab44544ee1d..4b834868e0ec7 100644
--- a/misc/arpd.c
+++ b/misc/arpd.c
@@ -664,6 +664,7 @@ int main(int argc, char **argv)
struct ifreq ifr = {};
for (i = 0; i < ifnum; i++) {
+ assert_valid_dev_name(ifnames[i], ifnames[i]);
strncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);
if (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {
perror("ioctl(SIOCGIFINDEX)");
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 19/51] lib/fs: Fix and simplify make_path()
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Calling stat() before mkdir() is racey: The entry might change in
between. Also, the call to stat() seems to exist only to check if the
directory exists already. So simply call mkdir() unconditionally and
catch only errors other than EEXIST.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
lib/fs.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/lib/fs.c b/lib/fs.c
index 1ff881ecfcd8c..ebe05cd44e11b 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -102,7 +102,6 @@ out:
int make_path(const char *path, mode_t mode)
{
char *dir, *delim;
- struct stat sbuf;
int rc = -1;
delim = dir = strdup(path);
@@ -120,20 +119,11 @@ int make_path(const char *path, mode_t mode)
if (delim)
*delim = '\0';
- if (stat(dir, &sbuf) != 0) {
- if (errno != ENOENT) {
- fprintf(stderr,
- "stat failed for %s: %s\n",
- dir, strerror(errno));
- goto out;
- }
-
- if (mkdir(dir, mode) != 0) {
- fprintf(stderr,
- "mkdir failed for %s: %s\n",
- dir, strerror(errno));
- goto out;
- }
+ rc = mkdir(dir, mode);
+ if (mkdir(dir, mode) != 0 && errno != EEXIST) {
+ fprintf(stderr, "mkdir failed for %s: %s\n",
+ dir, strerror(errno));
+ goto out;
}
if (delim == NULL)
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 08/51] ipntable: No need to check and assign to parms_rta
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
This variable is initialized at declaration and nowhere else does any
assignment to it happen, so just drop the check.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/ipntable.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..1837909fa42e7 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c
@@ -202,8 +202,6 @@ static int ipntable_modify(int cmd, int flags, int argc, char **argv)
if (get_u32(&queue, *argv, 0))
invarg("\"queue\" value is invalid", *argv);
- if (!parms_rta)
- parms_rta = (struct rtattr *)&parms_buf;
rta_addattr32(parms_rta, sizeof(parms_buf),
NDTPA_QUEUE_LEN, queue);
parms_change = 1;
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 18/51] lib/fs: Fix format string in find_fs_mount()
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
A field width of 4096 allows fscanf() to store that amount of characters
into the given buffer, though that doesn't include the terminating NULL
byte. Decrease the value by one to leave space for it.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
lib/fs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/fs.c b/lib/fs.c
index c59ac564581d0..1ff881ecfcd8c 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -45,7 +45,7 @@ static char *find_fs_mount(const char *fs_to_find)
return NULL;
}
- while (fscanf(fp, "%*s %4096s %127s %*s %*d %*d\n",
+ while (fscanf(fp, "%*s %4095s %127s %*s %*d %*d\n",
path, fstype) == 2) {
if (strcmp(fstype, fs_to_find) == 0) {
mnt = strdup(path);
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 04/51] ipaddress: Avoid accessing uninitialized variable lcl
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
If no address was given, ipaddr_modify() accesses uninitialized data
when assigning to req.ifa.ifa_prefixlen.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/ipaddress.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 3c9decb51b412..9307c9416dde3 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1888,7 +1888,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
char *lcl_arg = NULL;
char *valid_lftp = NULL;
char *preferred_lftp = NULL;
- inet_prefix lcl;
+ inet_prefix lcl = {};
inet_prefix peer;
int local_len = 0;
int peer_len = 0;
--
2.13.1
^ permalink raw reply related
* [iproute PATCH 33/51] ss: Don't leak fd in tcp_show_netlink_file()
From: Phil Sutter @ 2017-08-12 12:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-1-phil@nwl.cc>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
misc/ss.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index 4d2f75b571ea6..cda5e3b6a2d6f 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2687,41 +2687,44 @@ static int tcp_show_netlink_file(struct filter *f)
{
FILE *fp;
char buf[16384];
+ int err = -1;
if ((fp = fopen(getenv("TCPDIAG_FILE"), "r")) == NULL) {
perror("fopen($TCPDIAG_FILE)");
- return -1;
+ return err;
}
while (1) {
- int status, err;
+ int status, err2;
struct nlmsghdr *h = (struct nlmsghdr *)buf;
struct sockstat s = {};
status = fread(buf, 1, sizeof(*h), fp);
if (status < 0) {
perror("Reading header from $TCPDIAG_FILE");
- return -1;
+ break;
}
if (status != sizeof(*h)) {
perror("Unexpected EOF reading $TCPDIAG_FILE");
- return -1;
+ break;
}
status = fread(h+1, 1, NLMSG_ALIGN(h->nlmsg_len-sizeof(*h)), fp);
if (status < 0) {
perror("Reading $TCPDIAG_FILE");
- return -1;
+ break;
}
if (status + sizeof(*h) < h->nlmsg_len) {
perror("Unexpected EOF reading $TCPDIAG_FILE");
- return -1;
+ break;
}
/* The only legal exit point */
- if (h->nlmsg_type == NLMSG_DONE)
- return 0;
+ if (h->nlmsg_type == NLMSG_DONE) {
+ err = 0;
+ break;
+ }
if (h->nlmsg_type == NLMSG_ERROR) {
struct nlmsgerr *err = (struct nlmsgerr *)NLMSG_DATA(h);
@@ -2732,7 +2735,7 @@ static int tcp_show_netlink_file(struct filter *f)
errno = -err->error;
perror("TCPDIAG answered");
}
- return -1;
+ break;
}
parse_diag_msg(h, &s);
@@ -2741,10 +2744,15 @@ static int tcp_show_netlink_file(struct filter *f)
if (f && f->f && run_ssfilter(f->f, &s) == 0)
continue;
- err = inet_show_sock(h, &s);
- if (err < 0)
- return err;
+ err2 = inet_show_sock(h, &s);
+ if (err2 < 0) {
+ err = err2;
+ break;
+ }
}
+
+ fclose(fp);
+ return err;
}
static int tcp_show(struct filter *f, int socktype)
--
2.13.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox