* [PATCH net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs
From: Vivien Didelot @ 2017-09-26 17:48 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
mv88e6xxx_g2_irq_free locks the registers mutex, but not
mv88e6xxx_g1_irq_free, which results in a stack trace from
assert_reg_lock when unloading the mv88e6xxx module. Fix this.
Fixes: 3460a5770ce9 ("net: dsa: mv88e6xxx: Mask g1 interrupts and free interrupt")
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c6678aa9b4ef..b4359e4e5165 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -338,9 +338,11 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
int irq, virq;
u16 mask;
+ mutex_lock(&chip->reg_lock);
mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &mask);
mask |= GENMASK(chip->g1_irq.nirqs, 0);
mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask);
+ mutex_unlock(&chip->reg_lock);
free_irq(chip->irq, chip);
--
2.14.1
^ permalink raw reply related
* Re: [PATCH v2 00/16] Thunderbolt networking
From: Andy Shevchenko @ 2017-09-26 17:37 UTC (permalink / raw)
To: Mika Westerberg, Greg Kroah-Hartman, David S . Miller
Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat, Amir Levy,
Mario.Limonciello, Lukas Wunner, Andrew Lunn, linux-kernel,
netdev
In-Reply-To: <20170925110738.68382-1-mika.westerberg@linux.intel.com>
On Mon, 2017-09-25 at 14:07 +0300, Mika Westerberg wrote:
> Hi all,
>
> In addition of tunneling PCIe, Display Port and USB traffic,
> Thunderbolt
> allows connecting two hosts (domains) over a Thunderbolt cable. It is
> possible to tunnel arbitrary data packets over such connection using
> high-speed DMA rings available in the Thunderbolt host controller.
>
> In order to discover Thunderbolt services the other host supports,
> there is
> a software protocol running on top of the automatically configured
> control
> channel (ring 0). This protocol is called XDomain discovery protocol
> and it
> uses XDomain properties to describe the host (domain) and the services
> it
> supports.
>
> Once both sides have agreed what services are supported they can
> enable
> high-speed DMA rings to transfer data over the cable.
>
> This series adds support for the XDomain protocol so that we expose
> each
> remote connection as Thunderbolt XDomain device and each service as
> Thunderbolt service device. On top of that we create an API that
> allows
> writing drivers for these services and finally we provide an example
> Thunderbolt service driver that creates virtual ethernet inferface
> that
> allows tunneling networking packets over Thunderbolt cable. The API
> could
> be used for creating other Thunderbolt services, such as tunneling
> SCSI for
> example.
>
> The XDomain protocol and networking support is also available in macOS
> and
> Windows so this makes it possible to connect Linux to macOS and
> Windows as
> well.
>
> The patches are based on previous Thunderbolt networking patch series
> by
> Amir Levy and Michael Jamet, that can be found here:
>
> https://lwn.net/Articles/705998/
>
> The main difference to that patch series is that we have the XDomain
> protocol running in the kernel now so there is no need for a separate
> userspace daemon.
>
> Note this does not affect the existing functionality, so security
> levels
> and NVM firmware upgrade continue to work as before (with the small
> exception that now sysfs also shows the XDomain connections and
> services in
> addition to normal Thunderbolt devices). It is also possible to
> connect up
> to 5 Thunderbolt devices and then another host, and the network driver
> works exactly the same.
>
> This is second version of the patch series. The previous version (v1)
> can
> be found here:
>
> https://lwn.net/Articles/734019/
>
> Changes from the v1:
>
> * Add include/linux/thunderbolt.h to MAINTAINERS
> * Correct Linux version and date of new sysfs entries in
> Documentation/ABI/testing/sysfs-bus-thunderbolt
> * Move network driver from drivers/thunderbolt/net.c to
> drivers/net/thunderbolt.c and update it to follow coding style in
> drivers/net/*.
> * Add MAINTAINERS entry for the network driver
> * Minor cleanups
>
> In case someone wants to try this out, patch [16/16] adds
> documentation how
> the networking driver can be used. In short, if you connect Linux to a
> macOS or Windows, everything is done automatically (as those systems
> have
> the networking service enabled by default). For Linux to Linux
> connection
> one host needs to load the networking driver first (so that the other
> side
> can locate the networking service and load the corresponding driver).
Looks awesome!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
(I told privately to Mika about some minors and we agreed with him that
they will be considered only if there will be more comments on the
series)
>
> Amir Levy (1):
> net: Add support for networking over Thunderbolt cable
>
> Mika Westerberg (15):
> byteorder: Move {cpu_to_be32,be32_to_cpu}_array() from Thunderbolt
> to core
> thunderbolt: Add support for XDomain properties
> thunderbolt: Move enum tb_cfg_pkg_type to thunderbolt.h
> thunderbolt: Move thunderbolt domain structure to thunderbolt.h
> thunderbolt: Move tb_switch_phy_port_from_link() to thunderbolt.h
> thunderbolt: Add support for XDomain discovery protocol
> thunderbolt: Configure interrupt throttling for all interrupts
> thunderbolt: Add support for frame mode
> thunderbolt: Export ring handling functions to modules
> thunderbolt: Move ring descriptor flags to thunderbolt.h
> thunderbolt: Use spinlock in ring serialization
> thunderbolt: Use spinlock in NHI serialization
> thunderbolt: Add polling mode for rings
> thunderbolt: Add function to retrieve DMA device for the ring
> thunderbolt: Allocate ring HopID automatically if requested
>
> Documentation/ABI/testing/sysfs-bus-thunderbolt | 48 +
> Documentation/admin-guide/thunderbolt.rst | 24 +
> MAINTAINERS | 7 +
> drivers/net/Kconfig | 12 +
> drivers/net/Makefile | 3 +
> drivers/net/thunderbolt.c | 1379
> ++++++++++++++++++++
> drivers/thunderbolt/Makefile | 2 +-
> drivers/thunderbolt/ctl.c | 46 +-
> drivers/thunderbolt/ctl.h | 3 +-
> drivers/thunderbolt/domain.c | 197 ++-
> drivers/thunderbolt/icm.c | 218 +++-
> drivers/thunderbolt/nhi.c | 409 ++++--
> drivers/thunderbolt/nhi.h | 141 +-
> drivers/thunderbolt/nhi_regs.h | 11 +-
> drivers/thunderbolt/property.c | 670 ++++++++++
> drivers/thunderbolt/switch.c | 7 +-
> drivers/thunderbolt/tb.h | 88 +-
> drivers/thunderbolt/tb_msgs.h | 140 +-
> drivers/thunderbolt/xdomain.c | 1576
> +++++++++++++++++++++++
> include/linux/byteorder/generic.h | 16 +
> include/linux/mod_devicetable.h | 26 +
> include/linux/thunderbolt.h | 598 +++++++++
> scripts/mod/devicetable-offsets.c | 7 +
> scripts/mod/file2alias.c | 25 +
> 24 files changed, 5304 insertions(+), 349 deletions(-)
> create mode 100644 drivers/net/thunderbolt.c
> create mode 100644 drivers/thunderbolt/property.c
> create mode 100644 drivers/thunderbolt/xdomain.c
> create mode 100644 include/linux/thunderbolt.h
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH net v3] l2tp: fix race condition in l2tp_tunnel_delete
From: David Miller @ 2017-09-26 17:34 UTC (permalink / raw)
To: sd; +Cc: netdev, g.nault, lucien.xin, tparkin
In-Reply-To: <d6011f741f7ea38a61006b9f2cde8e544b4d206b.1506432922.git.sd@queasysnail.net>
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Tue, 26 Sep 2017 16:16:43 +0200
> If we try to delete the same tunnel twice, the first delete operation
> does a lookup (l2tp_tunnel_get), finds the tunnel, calls
> l2tp_tunnel_delete, which queues it for deletion by
> l2tp_tunnel_del_work.
>
> The second delete operation also finds the tunnel and calls
> l2tp_tunnel_delete. If the workqueue has already fired and started
> running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> same tunnel a second time, and try to free the socket again.
>
> Add a dead flag to prevent firing the workqueue twice. Then we can
> remove the check of queue_work's result that was meant to prevent that
> race but doesn't.
>
> Reproducer:
>
> ip l2tp add tunnel tunnel_id 3000 peer_tunnel_id 4000 local 192.168.0.2 remote 192.168.0.1 encap udp udp_sport 5000 udp_dport 6000
> ip l2tp add session name l2tp1 tunnel_id 3000 session_id 1000 peer_session_id 2000
> ip link set l2tp1 up
> ip l2tp del tunnel tunnel_id 3000
> ip l2tp del tunnel tunnel_id 3000
>
> Fixes: f8ccac0e4493 ("l2tp: put tunnel socket release on a workqueue")
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH] net: stmmac: Meet alignment requirements for DMA
From: David Miller @ 2017-09-26 17:34 UTC (permalink / raw)
To: matt.redfearn
Cc: netdev, alexandre.torgue, peppe.cavallaro, linux-kernel,
linux-mips, paul.burton, james.hogan
In-Reply-To: <587dc9a8-b974-e222-95b4-93c2a8f2aba2@imgtec.com>
From: Matt Redfearn <matt.redfearn@imgtec.com>
Date: Tue, 26 Sep 2017 14:57:39 +0100
> Since the MIPS architecture requires software cache management,
> performing a dma_map_single(TO_DEVICE) will writeback and invalidate
> the cachelines for the required region. To comply with (our
> interpretation of) the DMA API documentation, the MIPS implementation
> expects a cacheline aligned & sized buffer, so that it can writeback a
> set of complete cachelines. Indeed a recent patch
> (https://patchwork.linux-mips.org/patch/14624/) causes the MIPS dma
> mapping code to emit warnings if the buffer it is requested to sync is
> not cachline aligned. This driver, as is, fails this test and causes
> thousands of warnings to be emitted.
For a device write, if the device does what it is told and does not
access bytes outside of the periphery of the DMA mapping, nothing bad
can happen.
When the DMA buffer is mapped, the cpu side cachelines are flushed (even
ones which are partially part of the requsted mapping, this is _fine_).
The device writes into only the bytes it was given access to, which
starts at the DMA address.
The unmap and/or sync operation after the DMA transfer needs to do
nothing on the cpu side since the map operation flushed the cpu side
caches already.
When the cpu reads, it will pull the cacheline from main memory and
see what the device wrote.
It's not like the device can put "garbage" into the bytes earlier in
the cacheline it was given partial access to.
A device read is similar, the cpu cachelines are written out to main
memory at map time and the device sees what it needs to see.
I want to be shown a real example where corruption or bad data can
occur when the DMA API is used correctly.
Other platforms write "complete cachelines" in order to implement
the cpu and/or host controller parts of the DMA API implementation.
I see nothing special about MIPS at all.
If you're given a partial cache line, you align the addresses such
that you flush every cache line contained within the provided address
range.
I really don't see what the problem is and why MIPS needs special
handling. You will have to give specific examples, step by step,
where things can go wrong before I will be able to consider your
changes.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] ldmvsw: Remove redundant unlikely()
From: David Miller @ 2017-09-26 17:23 UTC (permalink / raw)
To: tklauser; +Cc: netdev, shannon.nelson
In-Reply-To: <20170926131415.8649-1-tklauser@distanz.ch>
From: Tobias Klauser <tklauser@distanz.ch>
Date: Tue, 26 Sep 2017 15:14:15 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net/mlx5: Remove redundant unlikely()
From: David Miller @ 2017-09-26 17:23 UTC (permalink / raw)
To: tklauser-93Khv+1bN0NyDzI6CaY1VQ
Cc: saeedm-VPRAkNaXOzVWk0Htik3J/w, matanb-VPRAkNaXOzVWk0Htik3J/w,
leonro-VPRAkNaXOzVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170926131323.8527-1-tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
From: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
Date: Tue, 26 Sep 2017 15:13:23 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next] bnxt_en: Remove redundant unlikely()
From: David Miller @ 2017-09-26 17:23 UTC (permalink / raw)
To: tklauser; +Cc: michael.chan, netdev
In-Reply-To: <20170926131226.8423-1-tklauser@distanz.ch>
From: Tobias Klauser <tklauser@distanz.ch>
Date: Tue, 26 Sep 2017 15:12:26 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/6] bpf: add meta pointer for direct access
From: Andy Gospodarek @ 2017-09-26 17:21 UTC (permalink / raw)
To: Daniel Borkmann
Cc: davem, alexei.starovoitov, john.fastabend, peter.waskiewicz.jr,
jakub.kicinski, netdev, mchan
In-Reply-To: <59C94FF4.8070900@iogearbox.net>
On Mon, Sep 25, 2017 at 08:50:28PM +0200, Daniel Borkmann wrote:
> On 09/25/2017 08:10 PM, Andy Gospodarek wrote:
> [...]
> > First, thanks for this detailed description. It was helpful to read
> > along with the patches.
> >
> > My only concern about this area being generic is that you are now in a
> > state where any bpf program must know about all the bpf programs in the
> > receive pipeline before it can properly parse what is stored in the
> > meta-data and add it to an skb (or perform any other action).
> > Especially if each program adds it's own meta-data along the way.
> >
> > Maybe this isn't a big concern based on the number of users of this
> > today, but it just starts to seem like a concern as there are these
> > hints being passed between layers that are challenging to track due to a
> > lack of a standard format for passing data between.
>
> Btw, we do have similar kind of programmable scratch buffer also today
> wrt skb cb[] that you can program from tc side, the perf ring buffer,
> which doesn't have any fixed layout for the slots, or a per-cpu map
> where you can transfer data between tail calls for example, then tail
> calls themselves that need to coordinate, or simply mangling of packets
> itself if you will, but more below to your use case ...
>
> > The main reason I bring this up is that Michael and I had discussed and
> > designed a way for drivers to communicate between each other that rx
> > resources could be freed after a tx completion on an XDP_REDIRECT
> > action. Much like this code, it involved adding an new element to
> > struct xdp_md that could point to the important information. Now that
> > there is a generic way to handle this, it would seem nice to be able to
> > leverage it, but I'm not sure how reliable this meta-data area would be
> > without the ability to mark it in some manner.
> >
> > For additional background, the minimum amount of data needed in the case
> > Michael and I were discussing was really 2 words. One to serve as a
> > pointer to an rx_ring structure and one to have a counter to the rx
> > producer entry. This data could be acessed by the driver processing the
> > tx completions and callback to the driver that received the frame off the wire
> > to perform any needed processing. (For those curious this would also require a
> > new callback/netdev op to act on this data stored in the XDP buffer.)
>
> What you describe above doesn't seem to be fitting to the use-case of
> this set, meaning the area here is fully programmable out of the BPF
> program, the infrastructure you're describing is some sort of means of
> communication between drivers for the XDP_REDIRECT, and should be
> outside of the control of the BPF program to mangle.
OK, I understand that perspective. I think saying this is really meant
as a BPF<->BPF communication channel for now is fine.
> You could probably reuse the base infra here and make a part of that
> inaccessible for the program with some sort of a fixed layout, but I
> haven't seen your code yet to be able to fully judge. Intention here
> is to allow for programmability within the BPF prog in a generic way,
> such that based on the use-case it can be populated in specific ways
> and propagated to the skb w/o having to define a fixed layout and
> bloat xdp_buff all the way to an skb while still retaining all the
> flexibility.
Some level of reuse might be proper, but I'd rather it be explicit for
my use since it's not exclusively something that will need to be used by
a BPF prog, but rather the driver. I'll produce some patches this week
for reference.
^ permalink raw reply
* Re: [PATCH] vti: fix use after free in vti_tunnel_xmit/vti6_tnl_xmit
From: David Miller @ 2017-09-26 17:13 UTC (permalink / raw)
To: alexey.kodanev; +Cc: netdev, steffen.klassert, herbert
In-Reply-To: <1506428069-4716-1-git-send-email-alexey.kodanev@oracle.com>
From: Alexey Kodanev <alexey.kodanev@oracle.com>
Date: Tue, 26 Sep 2017 15:14:29 +0300
> When running LTP IPsec tests, KASan might report:
>
> BUG: KASAN: use-after-free in vti_tunnel_xmit+0xeee/0xff0 [ip_vti]
> Read of size 4 at addr ffff880dc6ad1980 by task swapper/0/0
> ...
...
> Can be fixed if we get skb->len before dst_output().
>
> Fixes: b9959fd3b0fa ("vti: switch to new ip tunnel code")
> Fixes: 22e1b23dafa8 ("vti6: Support inter address family tunneling.")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
Applied and queued up for -stable.
^ permalink raw reply
* [iproute PATCH v2 2/3] tc: flower: No need to cache indev arg
From: Phil Sutter @ 2017-09-26 16:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170926163548.24347-1-phil@nwl.cc>
Since addattrstrz() will copy the provided string into the attribute
payload, there is no need to cache the data.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
tc/f_flower.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tc/f_flower.c b/tc/f_flower.c
index 934832e2bbe90..99e62a382dec6 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -629,11 +629,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
} else if (matches(*argv, "skip_sw") == 0) {
flags |= TCA_CLS_FLAGS_SKIP_SW;
} else if (matches(*argv, "indev") == 0) {
- char ifname[IFNAMSIZ] = {};
-
NEXT_ARG();
- strncpy(ifname, *argv, sizeof(ifname) - 1);
- addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, ifname);
+ addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);
} else if (matches(*argv, "vlan_id") == 0) {
__u16 vid;
--
2.13.1
^ permalink raw reply related
* Re: [PATCH net-next] kcm: Remove redundant unlikely()
From: David Miller @ 2017-09-26 16:54 UTC (permalink / raw)
To: tklauser; +Cc: netdev
In-Reply-To: <20170926092258.21391-1-tklauser@distanz.ch>
From: Tobias Klauser <tklauser@distanz.ch>
Date: Tue, 26 Sep 2017 11:22:58 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: Remove redundant unlikely()
From: David Miller @ 2017-09-26 16:54 UTC (permalink / raw)
To: tklauser; +Cc: netdev, kuznet, yoshfuji
In-Reply-To: <20170926092231.21289-1-tklauser@distanz.ch>
From: Tobias Klauser <tklauser@distanz.ch>
Date: Tue, 26 Sep 2017 11:22:31 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] datagram: Remove redundant unlikely()
From: David Miller @ 2017-09-26 16:54 UTC (permalink / raw)
To: tklauser; +Cc: netdev
In-Reply-To: <20170926092142.21156-1-tklauser@distanz.ch>
From: Tobias Klauser <tklauser@distanz.ch>
Date: Tue, 26 Sep 2017 11:21:42 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: ena: Remove redundant unlikely()
From: David Miller @ 2017-09-26 16:54 UTC (permalink / raw)
To: tklauser; +Cc: netdev, netanel, saeed, zorik
In-Reply-To: <20170926090423.16937-1-tklauser@distanz.ch>
From: Tobias Klauser <tklauser@distanz.ch>
Date: Tue, 26 Sep 2017 11:04:23 +0200
> IS_ERR() already implies unlikely(), so it can be omitted.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Applied.
^ permalink raw reply
* Re: [PATCH v2] p54: don't unregister leds when they are not initialized
From: Christian Lamparter @ 2017-09-26 16:53 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Kalle Valo, linux-wireless, netdev, linux-kernel, Dmitry Vyukov,
Kostya Serebryany
In-Reply-To: <17c60ebcc8ce7f20de41a55087d24dfdfca09c67.1506438620.git.andreyknvl@google.com>
On Tuesday, September 26, 2017 5:11:33 PM CEST Andrey Konovalov wrote:
> ieee80211_register_hw() in p54_register_common() may fail and leds won't
> get initialized. Currently p54_unregister_common() doesn't check that and
> always calls p54_unregister_leds(). The fix is to check priv->registered
> flag before calling p54_unregister_leds().
>
> Found by syzkaller.
>
> [...]
> process_scheduled_works kernel/workqueue.c:2179
> worker_thread+0xb2b/0x1850 kernel/workqueue.c:2255
> kthread+0x3a1/0x470 kernel/kthread.c:231
> ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Cc: stable@vger.kernel.org
Acked-by: Christian Lamparter <chunkeey@googlemail.com>
Thanks for making the patch too!
^ permalink raw reply
* Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN
From: Andrey Ryabinin @ 2017-09-26 16:49 UTC (permalink / raw)
To: Arnd Bergmann, David Laight
Cc: Mauro Carvalho Chehab, Jiri Pirko, Arend van Spriel, Kalle Valo,
David S. Miller, Alexander Potapenko, Dmitry Vyukov,
Masahiro Yamada, Michal Marek, Andrew Morton, Kees Cook,
Geert Uytterhoeven, Greg Kroah-Hartman,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, linux-wireless@vger.kernel.org
In-Reply-To: <CAK8P3a37Ts5q7BvA2JWse87huyAp+=e18CUXEt8731RrBnB+Ow@mail.gmail.com>
On 09/26/2017 09:47 AM, Arnd Bergmann wrote:
> On Mon, Sep 25, 2017 at 11:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Sep 25, 2017 at 7:41 AM, David Laight <David.Laight@aculab.com> wrote:
>>> From: Arnd Bergmann
>>>> Sent: 22 September 2017 22:29
>>> ...
>>>> It seems that this is triggered in part by using strlcpy(), which the
>>>> compiler doesn't recognize as copying at most 'len' bytes, since strlcpy
>>>> is not part of the C standard.
>>>
>>> Neither is strncpy().
>>>
>>> It'll almost certainly be a marker in a header file somewhere,
>>> so it should be possibly to teach it about other functions.
>>
>> I'm currently travelling and haven't investigated in detail, but from
>> taking a closer look here, I found that the hardened 'strlcpy()'
>> in include/linux/string.h triggers it. There is also a hardened
>> (much shorted) 'strncpy()' that doesn't trigger it in the same file,
>> and having only the extern declaration of strncpy also doesn't.
>
> And a little more experimenting leads to this simple patch that fixes
> the problem:
>
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -254,7 +254,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const
> char *q, size_t size)
> size_t q_size = __builtin_object_size(q, 0);
> if (p_size == (size_t)-1 && q_size == (size_t)-1)
> return __real_strlcpy(p, q, size);
> - ret = strlen(q);
> + ret = __builtin_strlen(q);
I think this is not correct. Fortified strlen called here on purpose. If sizeof q is known at compile time
and 'q' contains not-null fortified strlen() will panic.
> if (size) {
> size_t len = (ret >= size) ? size - 1 : ret;
> if (__builtin_constant_p(len) && len >= p_size)
>
> The problem is apparently that the fortified strlcpy calls the fortified strlen,
> which in turn calls strnlen and that ends up calling the extern '__real_strnlen'
> that gcc cannot reduce to a constant expression for a constant input.
Per my observation, it's the code like this:
if ()
fortify_panic(__func__);
somehow prevent gcc to merge several "struct i2c_board_info info;" into one stack slot.
With the hack bellow, stack usage reduced to ~1,6K:
---
include/linux/string.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 54d21783e18d..9a96ff3ebf94 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -261,8 +261,6 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
if (p_size == (size_t)-1)
return __builtin_strlen(p);
ret = strnlen(p, p_size);
- if (p_size <= ret)
- fortify_panic(__func__);
return ret;
}
@@ -271,8 +269,6 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
{
size_t p_size = __builtin_object_size(p, 0);
__kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
- if (p_size <= ret && maxlen != ret)
- fortify_panic(__func__);
return ret;
}
> Not sure if that change is the best fix, but it seems to address the problem in
> this driver and probably leads to better code in other places as well.
>
Probably it would be better to solve this on the strlcpy side, but I haven't found the way to do this right.
Alternative solutions:
- use memcpy() instead of strlcpy(). All source strings are smaller than I2C_NAME_SIZE, so we could
do something like this - memcpy(info.type, "si2168", sizeof("si2168"));
Also this should be faster.
- Move code under different "case:" in the switch(dev->model) to the separate function should help as well.
But it might be harder to backport into stables.
^ permalink raw reply related
* Re: Can libpcap filter on vlan tags when vlans are hardware-accelerated?
From: Ben Greear @ 2017-09-26 16:41 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev
In-Reply-To: <20170912202617.ny3bbi2dthxxffh3@unicorn.suse.cz>
On 09/12/2017 01:26 PM, Michal Kubecek wrote:
> On Tue, Sep 12, 2017 at 11:54:43AM -0700, Ben Greear wrote:
>> It does not appear to work on Fedora-26, and I'm curious if someone
>> knows what needs doing to get this support working?
>
> It's rather complicated. The "vlan" and "vlan <id>" filters didn't
> handle the case when vlan information is passed in metadata until commit
> 04660eb1e561 ("Use BPF extensions in compiled filters"), i.e. libpcap
> 1.7.0. Unfortunately that commit made libpcap always check only metadata
> for the first outermost vlan tag so that it broke the case when vlan
> information is passed in packet itself (which is less frequent today).
>
> To handle both cases correctly, you would need libpcap with commits
> d739b068ac29 ("Make VLAN filter handle both metadata and inline tags")
> and 7c7a19fbd9af ("Fix logic of combined VLAN test") and also the
> optimizer fix from
>
> https://github.com/the-tcpdump-group/libpcap/pull/582/commits/075015a3d17a
>
> (without it the filters generate incorrect BPF in some cases unless the
> optimizer is disabled). As far as I can see, these commits are not in
> any release yet.
>
> Michal Kubecek
>
So, I cloned the latest libpcap, and I'm going to start poking at this.
Do you happen to know if I need to do anything special other than
'pcap_compile()'? I'm curious how the library would know if it can use
newer kernel API or not...or maybe it is somehow magically backwards/forward
compatible?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* [iproute PATCH v2 0/3] Check user supplied interface name lengths
From: Phil Sutter @ 2017-09-26 16:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
This series adds explicit checks for user-supplied interface names to
make sure their length fits Linux's requirements.
The first two patches simplify interface name parsing in some places -
these are side-effects of working on the actual implementation provided
in patch three.
Changes since v1:
- Patches 1 and 2 introduced.
- Changes to patch 3 are listed in there.
Phil Sutter (3):
ip{6,}tunnel: Avoid copying user-supplied interface name around
tc: flower: No need to cache indev arg
Check user supplied interface name lengths
include/utils.h | 1 +
ip/ip6tunnel.c | 9 +++++----
ip/ipl2tp.c | 3 ++-
ip/iplink.c | 27 ++++++++-------------------
ip/ipmaddr.c | 1 +
ip/iprule.c | 4 ++++
ip/iptunnel.c | 27 +++++++++++++--------------
ip/iptuntap.c | 4 +++-
lib/utils.c | 10 ++++++++++
misc/arpd.c | 1 +
tc/f_flower.c | 6 ++----
11 files changed, 50 insertions(+), 43 deletions(-)
--
2.13.1
^ permalink raw reply
* [iproute PATCH v2 3/3] Check user supplied interface name lengths
From: Phil Sutter @ 2017-09-26 16:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170926163548.24347-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 check_ifname() to all places where
user supplied interface name is parsed. In many cases, the interface
must exist already and is therefore looked up using ll_name_to_index(),
so if_nametoindex() will perform the necessary checks already.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- added missing check to tc/f_flower.c
- Drop some useless checks from ip/ip{6,}tunnel.c (ll_name_to_index()
will detect illegal interface names for us).
- Renamed assert_valid_dev_name() to the shorter check_ifname().
- iplink: Check 'name' and 'dev' parameters right where they are parsed.
- ipl2tp: Drop needless check for p->ifname[0].
---
include/utils.h | 1 +
ip/ip6tunnel.c | 3 ++-
ip/ipl2tp.c | 3 ++-
ip/iplink.c | 27 ++++++++-------------------
ip/ipmaddr.c | 1 +
ip/iprule.c | 4 ++++
ip/iptunnel.c | 5 ++++-
ip/iptuntap.c | 4 +++-
lib/utils.c | 10 ++++++++++
misc/arpd.c | 1 +
tc/f_flower.c | 1 +
11 files changed, 37 insertions(+), 23 deletions(-)
diff --git a/include/utils.h b/include/utils.h
index c9ed230b96044..12f0735e8aa0d 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -133,6 +133,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 check_ifname(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 c12d700e74189..2c10b9c3fa7b5 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -273,7 +273,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);
+ check_ifname("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..06f1bd064c914 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -182,7 +182,7 @@ static int create_session(struct l2tp_parm *p)
if (p->peer_cookie_len)
addattr_l(&req.n, 1024, L2TP_ATTR_PEER_COOKIE,
p->peer_cookie, p->peer_cookie_len);
- if (p->ifname && p->ifname[0])
+ if (p->ifname)
addattrstrz(&req.n, 1024, L2TP_ATTR_IFNAME, p->ifname);
if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
@@ -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();
+ check_ifname("name", *argv);
p->ifname = *argv;
} else if (strcmp(*argv, "remote") == 0) {
NEXT_ARG();
diff --git a/ip/iplink.c b/ip/iplink.c
index ff5b56c038d28..f000a7992c92e 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -573,6 +573,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
req->i.ifi_flags &= ~IFF_UP;
} else if (strcmp(*argv, "name") == 0) {
NEXT_ARG();
+ check_ifname("name", *argv);
*name = *argv;
} else if (strcmp(*argv, "index") == 0) {
NEXT_ARG();
@@ -848,6 +849,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
NEXT_ARG();
if (*dev)
duparg2("dev", *argv);
+ check_ifname("dev", *argv);
*dev = *argv;
dev_index = ll_name_to_index(*dev);
}
@@ -870,7 +872,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;
@@ -960,13 +961,8 @@ 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);
+ addattr_l(&req.n, sizeof(req),
+ IFLA_IFNAME, name, strlen(name) + 1);
}
if (type) {
@@ -1016,7 +1012,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,
@@ -1029,13 +1024,8 @@ 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);
+ addattr_l(&req.n, sizeof(req),
+ IFLA_IFNAME, name, strlen(name) + 1);
}
addattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);
@@ -1265,6 +1255,7 @@ static int do_set(int argc, char **argv)
flags &= ~IFF_UP;
} else if (strcmp(*argv, "name") == 0) {
NEXT_ARG();
+ check_ifname("name", newname);
newname = *argv;
} else if (matches(*argv, "address") == 0) {
NEXT_ARG();
@@ -1355,6 +1346,7 @@ static int do_set(int argc, char **argv)
if (dev)
duparg2("dev", *argv);
+ check_ifname("dev", dev);
dev = *argv;
}
argc--; argv++;
@@ -1383,9 +1375,6 @@ 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");
if (do_changename(dev, newname) < 0)
return -1;
dev = newname;
diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 85a69e779563d..282a06153c79a 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);
+ check_ifname("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..33cfd87195212 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();
+ check_ifname("iif", *argv);
strncpy(filter.iif, *argv, IFNAMSIZ);
filter.iifmask = 1;
} else if (strcmp(*argv, "oif") == 0) {
NEXT_ARG();
+ check_ifname("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();
+ check_ifname("dev/iif", *argv);
addattr_l(&req.n, sizeof(req), FRA_IFNAME,
*argv, strlen(*argv)+1);
} else if (strcmp(*argv, "oif") == 0) {
NEXT_ARG();
+ check_ifname("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 0acfd0793d3cd..851a80aad73a9 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -178,7 +178,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);
+ check_ifname("name", *argv);
+ strncpy(p->name, *argv, IFNAMSIZ);
if (cmd == SIOCCHGTUNNEL && count == 0) {
struct ip_tunnel_parm old_p = {};
@@ -487,6 +488,7 @@ static int do_prl(int argc, char **argv)
count++;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
+ check_ifname("dev", *argv);
medium = *argv;
} else {
fprintf(stderr,
@@ -534,6 +536,7 @@ static int do_6rd(int argc, char **argv)
cmd = SIOCDEL6RD;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
+ check_ifname("dev", *argv);
medium = *argv;
} else {
fprintf(stderr,
diff --git a/ip/iptuntap.c b/ip/iptuntap.c
index 451f7f0eac6bb..4400dc2fa2a88 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);
+ check_ifname("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);
+ check_ifname("name", *argv);
strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
}
count++;
diff --git a/lib/utils.c b/lib/utils.c
index bbd3cbc46a0e5..c4a02b8f9f52a 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -699,6 +699,16 @@ void duparg2(const char *key, const char *arg)
exit(-1);
}
+void check_ifname(const char *arg, const char *name)
+{
+ size_t len = strlen(name);
+
+ if (!len)
+ invarg("Empty interface name not allowed.", arg);
+ if (len >= IFNAMSIZ)
+ invarg("Interface name is too long.", name);
+}
+
int matches(const char *cmd, const char *pattern)
{
int len = strlen(cmd);
diff --git a/misc/arpd.c b/misc/arpd.c
index bfab44544ee1d..d42df9e58a9f1 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++) {
+ check_ifname(ifnames[i], ifnames[i]);
strncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);
if (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {
perror("ioctl(SIOCGIFINDEX)");
diff --git a/tc/f_flower.c b/tc/f_flower.c
index 99e62a382dec6..ff45ea7af086e 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -630,6 +630,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
flags |= TCA_CLS_FLAGS_SKIP_SW;
} else if (matches(*argv, "indev") == 0) {
NEXT_ARG();
+ check_ifname("indev", *argv);
addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);
} else if (matches(*argv, "vlan_id") == 0) {
__u16 vid;
--
2.13.1
^ permalink raw reply related
* [iproute PATCH v2 1/3] ip{6,}tunnel: Avoid copying user-supplied interface name around
From: Phil Sutter @ 2017-09-26 16:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170926163548.24347-1-phil@nwl.cc>
In both files' parse_args() functions as well as in iptunnel's do_prl()
and do_6rd() functions, a user-supplied 'dev' parameter is uselessly
copied into a temporary buffer before passing it to ll_name_to_index()
or copying into a struct ifreq. Avoid this by just caching the argv
pointer value until the later lookup/strcpy.
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
ip/ip6tunnel.c | 6 +++---
ip/iptunnel.c | 22 +++++++++-------------
2 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index b4a7def144226..c12d700e74189 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -136,7 +136,7 @@ static void print_tunnel(struct ip6_tnl_parm2 *p)
static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
{
int count = 0;
- char medium[IFNAMSIZ] = {};
+ const char *medium = NULL;
while (argc > 0) {
if (strcmp(*argv, "mode") == 0) {
@@ -180,7 +180,7 @@ 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);
+ medium = *argv;
} else if (strcmp(*argv, "encaplimit") == 0) {
NEXT_ARG();
if (strcmp(*argv, "none") == 0) {
@@ -285,7 +285,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
count++;
argc--; argv++;
}
- if (medium[0]) {
+ if (medium) {
p->link = ll_name_to_index(medium);
if (p->link == 0) {
fprintf(stderr, "Cannot find device \"%s\"\n", medium);
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 105d0f5576f1a..0acfd0793d3cd 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -60,7 +60,7 @@ static void set_tunnel_proto(struct ip_tunnel_parm *p, int proto)
static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
{
int count = 0;
- char medium[IFNAMSIZ] = {};
+ const char *medium = NULL;
int isatap = 0;
memset(p, 0, sizeof(*p));
@@ -139,7 +139,7 @@ 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);
+ medium = *argv;
} else if (strcmp(*argv, "ttl") == 0 ||
strcmp(*argv, "hoplimit") == 0 ||
strcmp(*argv, "hlim") == 0) {
@@ -216,7 +216,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
}
}
- if (medium[0]) {
+ if (medium) {
p->link = ll_name_to_index(medium);
if (p->link == 0) {
fprintf(stderr, "Cannot find device \"%s\"\n", medium);
@@ -465,9 +465,8 @@ static int do_prl(int argc, char **argv)
{
struct ip_tunnel_prl p = {};
int count = 0;
- int devname = 0;
int cmd = 0;
- char medium[IFNAMSIZ] = {};
+ const char *medium = NULL;
while (argc > 0) {
if (strcmp(*argv, "prl-default") == 0) {
@@ -488,8 +487,7 @@ static int do_prl(int argc, char **argv)
count++;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ-1);
- devname++;
+ medium = *argv;
} else {
fprintf(stderr,
"Invalid PRL parameter \"%s\"\n", *argv);
@@ -502,7 +500,7 @@ static int do_prl(int argc, char **argv)
}
argc--; argv++;
}
- if (devname == 0) {
+ if (!medium) {
fprintf(stderr, "Must specify device\n");
exit(-1);
}
@@ -513,9 +511,8 @@ static int do_prl(int argc, char **argv)
static int do_6rd(int argc, char **argv)
{
struct ip_tunnel_6rd ip6rd = {};
- int devname = 0;
int cmd = 0;
- char medium[IFNAMSIZ] = {};
+ const char *medium = NULL;
inet_prefix prefix;
while (argc > 0) {
@@ -537,8 +534,7 @@ static int do_6rd(int argc, char **argv)
cmd = SIOCDEL6RD;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ-1);
- devname++;
+ medium = *argv;
} else {
fprintf(stderr,
"Invalid 6RD parameter \"%s\"\n", *argv);
@@ -546,7 +542,7 @@ static int do_6rd(int argc, char **argv)
}
argc--; argv++;
}
- if (devname == 0) {
+ if (!medium) {
fprintf(stderr, "Must specify device\n");
exit(-1);
}
--
2.13.1
^ permalink raw reply related
* [PATCH v3] ebtables: fix race condition in frame_filter_net_init()
From: Artem Savkov @ 2017-09-26 16:35 UTC (permalink / raw)
To: Florian Westphal
Cc: Pablo Neira Ayuso, netdev, linux-kernel, netfilter-devel,
Artem Savkov
In-Reply-To: <20170926153923.30094-1-asavkov@redhat.com>
It is possible for ebt_in_hook to be triggered before ebt_table is assigned
resulting in a NULL-pointer dereference. Make sure hooks are
registered as the last step.
v3: restore errorneously removed ops == NULL case check
Fixes: aee12a0a3727 ebtables: remove nf_hook_register usage
Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
include/linux/netfilter_bridge/ebtables.h | 7 ++++---
net/bridge/netfilter/ebtable_broute.c | 4 ++--
net/bridge/netfilter/ebtable_filter.c | 4 ++--
net/bridge/netfilter/ebtable_nat.c | 4 ++--
net/bridge/netfilter/ebtables.c | 17 +++++++++--------
5 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/include/linux/netfilter_bridge/ebtables.h b/include/linux/netfilter_bridge/ebtables.h
index 2c2a5514b0df..528b24c78308 100644
--- a/include/linux/netfilter_bridge/ebtables.h
+++ b/include/linux/netfilter_bridge/ebtables.h
@@ -108,9 +108,10 @@ struct ebt_table {
#define EBT_ALIGN(s) (((s) + (__alignof__(struct _xt_align)-1)) & \
~(__alignof__(struct _xt_align)-1))
-extern struct ebt_table *ebt_register_table(struct net *net,
- const struct ebt_table *table,
- const struct nf_hook_ops *);
+extern int ebt_register_table(struct net *net,
+ const struct ebt_table *table,
+ const struct nf_hook_ops *ops,
+ struct ebt_table **res);
extern void ebt_unregister_table(struct net *net, struct ebt_table *table,
const struct nf_hook_ops *);
extern unsigned int ebt_do_table(struct sk_buff *skb,
diff --git a/net/bridge/netfilter/ebtable_broute.c b/net/bridge/netfilter/ebtable_broute.c
index 2585b100ebbb..276b60262981 100644
--- a/net/bridge/netfilter/ebtable_broute.c
+++ b/net/bridge/netfilter/ebtable_broute.c
@@ -65,8 +65,8 @@ static int ebt_broute(struct sk_buff *skb)
static int __net_init broute_net_init(struct net *net)
{
- net->xt.broute_table = ebt_register_table(net, &broute_table, NULL);
- return PTR_ERR_OR_ZERO(net->xt.broute_table);
+ return ebt_register_table(net, &broute_table, NULL,
+ &net->xt.broute_table);
}
static void __net_exit broute_net_exit(struct net *net)
diff --git a/net/bridge/netfilter/ebtable_filter.c b/net/bridge/netfilter/ebtable_filter.c
index 45a00dbdbcad..c41da5fac84f 100644
--- a/net/bridge/netfilter/ebtable_filter.c
+++ b/net/bridge/netfilter/ebtable_filter.c
@@ -93,8 +93,8 @@ static const struct nf_hook_ops ebt_ops_filter[] = {
static int __net_init frame_filter_net_init(struct net *net)
{
- net->xt.frame_filter = ebt_register_table(net, &frame_filter, ebt_ops_filter);
- return PTR_ERR_OR_ZERO(net->xt.frame_filter);
+ return ebt_register_table(net, &frame_filter, ebt_ops_filter,
+ &net->xt.frame_filter);
}
static void __net_exit frame_filter_net_exit(struct net *net)
diff --git a/net/bridge/netfilter/ebtable_nat.c b/net/bridge/netfilter/ebtable_nat.c
index 57cd5bb154e7..08df7406ecb3 100644
--- a/net/bridge/netfilter/ebtable_nat.c
+++ b/net/bridge/netfilter/ebtable_nat.c
@@ -93,8 +93,8 @@ static const struct nf_hook_ops ebt_ops_nat[] = {
static int __net_init frame_nat_net_init(struct net *net)
{
- net->xt.frame_nat = ebt_register_table(net, &frame_nat, ebt_ops_nat);
- return PTR_ERR_OR_ZERO(net->xt.frame_nat);
+ return ebt_register_table(net, &frame_nat, ebt_ops_nat,
+ &net->xt.frame_nat);
}
static void __net_exit frame_nat_net_exit(struct net *net)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 83951f978445..3b3dcf719e07 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1169,9 +1169,8 @@ static void __ebt_unregister_table(struct net *net, struct ebt_table *table)
kfree(table);
}
-struct ebt_table *
-ebt_register_table(struct net *net, const struct ebt_table *input_table,
- const struct nf_hook_ops *ops)
+int ebt_register_table(struct net *net, const struct ebt_table *input_table,
+ const struct nf_hook_ops *ops, struct ebt_table **res)
{
struct ebt_table_info *newinfo;
struct ebt_table *t, *table;
@@ -1183,7 +1182,7 @@ ebt_register_table(struct net *net, const struct ebt_table *input_table,
repl->entries == NULL || repl->entries_size == 0 ||
repl->counters != NULL || input_table->private != NULL) {
BUGPRINT("Bad table data for ebt_register_table!!!\n");
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
}
/* Don't add one table to multiple lists. */
@@ -1252,16 +1251,18 @@ ebt_register_table(struct net *net, const struct ebt_table *input_table,
list_add(&table->list, &net->xt.tables[NFPROTO_BRIDGE]);
mutex_unlock(&ebt_mutex);
+ WRITE_ONCE(*res, table);
+
if (!ops)
- return table;
+ return 0;
ret = nf_register_net_hooks(net, ops, hweight32(table->valid_hooks));
if (ret) {
__ebt_unregister_table(net, table);
- return ERR_PTR(ret);
+ *res = NULL;
}
- return table;
+ return ret;
free_unlock:
mutex_unlock(&ebt_mutex);
free_chainstack:
@@ -1276,7 +1277,7 @@ ebt_register_table(struct net *net, const struct ebt_table *input_table,
free_table:
kfree(table);
out:
- return ERR_PTR(ret);
+ return ret;
}
void ebt_unregister_table(struct net *net, struct ebt_table *table,
--
2.13.5
^ permalink raw reply related
* Re: [PATCH] lib: fix multiple strlcpy definition
From: Baruch Siach @ 2017-09-26 16:27 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger, netdev
In-Reply-To: <20170926155524.GA26610@orbyte.nwl.cc>
Hi Phil,
On Tue, Sep 26, 2017 at 05:55:24PM +0200, Phil Sutter wrote:
> On Tue, Sep 26, 2017 at 02:08:49PM +0300, Baruch Siach wrote:
> [...]
> > diff --git a/configure b/configure
> > index 7be8fb113cc9..787b2e061af9 100755
> > --- a/configure
> > +++ b/configure
> > @@ -326,6 +326,27 @@ EOF
> > rm -f $TMPDIR/dbtest.c $TMPDIR/dbtest
> > }
> >
> > +check_strlcpy()
> > +{
> > + cat >$TMPDIR/strtest.c <<EOF
> > +#include <string.h>
> > +int main(int argc, char **argv) {
> > + char dst[10];
> > + strlcpy("test", dst, sizeof(dst));
>
> You swapped source and destination here. It's not important for the
> given use-case, but the resulting binary should segfault.
Will fix that in v2.
> Apart from that, LGTM!
Thanks. I'll take this as an ack.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply
* [PATCH net] packet: only test po->has_vnet_hdr once in packet_snd
From: Willem de Bruijn @ 2017-09-26 16:20 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Packet socket option po->has_vnet_hdr can be updated concurrently with
other operations if no ring is attached.
Do not test the option twice in packet_snd, as the value may change in
between calls. A race on setsockopt disable may cause a packet > mtu
to be sent without having GSO options set.
Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.")
Signed-off-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
net/packet/af_packet.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index d288f52c53f7..1da0851f51f2 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2840,6 +2840,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
struct virtio_net_hdr vnet_hdr = { 0 };
int offset = 0;
struct packet_sock *po = pkt_sk(sk);
+ bool has_vnet_hdr = false;
int hlen, tlen, linear;
int extra_len = 0;
@@ -2883,6 +2884,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
err = packet_snd_vnet_parse(msg, &len, &vnet_hdr);
if (err)
goto out_unlock;
+ has_vnet_hdr = true;
}
if (unlikely(sock_flag(sk, SOCK_NOFCS))) {
@@ -2941,7 +2943,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
skb->priority = sk->sk_priority;
skb->mark = sockc.mark;
- if (po->has_vnet_hdr) {
+ if (has_vnet_hdr) {
err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
if (err)
goto out_free;
--
2.14.1.821.g8fa685d3b7-goog
^ permalink raw reply related
* [PATCH net] packet: in packet_do_bind, test fanout with bind_lock held
From: Willem de Bruijn @ 2017-09-26 16:19 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Once a socket has po->fanout set, it remains a member of the group
until it is destroyed. The prot_hook must be constant and identical
across sockets in the group.
If fanout_add races with packet_do_bind between the test of po->fanout
and taking the lock, the bind call may make type or dev inconsistent
with that of the fanout group.
Hold po->bind_lock when testing po->fanout to avoid this race.
I had to introduce artificial delay (local_bh_enable) to actually
observe the race.
Fixes: dc99f600698d ("packet: Add fanout support.")
Signed-off-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
net/packet/af_packet.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1da0851f51f2..bec01a3daf5b 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3071,13 +3071,15 @@ static int packet_do_bind(struct sock *sk, const char *name, int ifindex,
int ret = 0;
bool unlisted = false;
- if (po->fanout)
- return -EINVAL;
-
lock_sock(sk);
spin_lock(&po->bind_lock);
rcu_read_lock();
+ if (po->fanout) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
if (name) {
dev = dev_get_by_name_rcu(sock_net(sk), name);
if (!dev) {
--
2.14.1.821.g8fa685d3b7-goog
^ permalink raw reply related
* Re: [PATCH] lib: fix multiple strlcpy definition
From: Phil Sutter @ 2017-09-26 15:55 UTC (permalink / raw)
To: Baruch Siach; +Cc: Stephen Hemminger, netdev
In-Reply-To: <7f5fee59e46bfd3b4acf8ed9a8fbd8c7b4f1cd70.1506424129.git.baruch@tkos.co.il>
On Tue, Sep 26, 2017 at 02:08:49PM +0300, Baruch Siach wrote:
[...]
> diff --git a/configure b/configure
> index 7be8fb113cc9..787b2e061af9 100755
> --- a/configure
> +++ b/configure
> @@ -326,6 +326,27 @@ EOF
> rm -f $TMPDIR/dbtest.c $TMPDIR/dbtest
> }
>
> +check_strlcpy()
> +{
> + cat >$TMPDIR/strtest.c <<EOF
> +#include <string.h>
> +int main(int argc, char **argv) {
> + char dst[10];
> + strlcpy("test", dst, sizeof(dst));
You swapped source and destination here. It's not important for the
given use-case, but the resulting binary should segfault.
Apart from that, LGTM!
Cheers, Phil
^ permalink raw reply
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