* [PATCH] iscsi: respond to netlink with unicast when appropriate
From: Chris Leech @ 2018-04-09 22:15 UTC (permalink / raw)
To: linux-scsi, netdev, Lee Duncan, open-iscsi; +Cc: Chris Leech
Instead of always multicasting responses, send a unicast netlink message
directed at the correct pid. This will be needed if we ever want to
support multiple userspace processes interacting with the kernel over
iSCSI netlink simultaneously. Limitations can currently be seen if you
attempt to run multiple iscsistart commands in parallel.
We've fixed up the userspace issues in iscsistart that prevented
multiple instances from running, so now attempts to speed up booting by
bringing up multiple iscsi sessions at once in the initramfs are just
running into misrouted responses that this fixes.
Signed-off-by: Chris Leech <cleech@redhat.com>
---
drivers/scsi/scsi_transport_iscsi.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index f4b52b44b966..65f6c94f2e9b 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2322,6 +2322,12 @@ iscsi_multicast_skb(struct sk_buff *skb, uint32_t group, gfp_t gfp)
return nlmsg_multicast(nls, skb, 0, group, gfp);
}
+static int
+iscsi_unicast_skb(struct sk_buff *skb, u32 portid)
+{
+ return nlmsg_unicast(nls, skb, portid);
+}
+
int iscsi_recv_pdu(struct iscsi_cls_conn *conn, struct iscsi_hdr *hdr,
char *data, uint32_t data_size)
{
@@ -2524,14 +2530,11 @@ void iscsi_ping_comp_event(uint32_t host_no, struct iscsi_transport *transport,
EXPORT_SYMBOL_GPL(iscsi_ping_comp_event);
static int
-iscsi_if_send_reply(uint32_t group, int seq, int type, int done, int multi,
- void *payload, int size)
+iscsi_if_send_reply(u32 portid, int type, void *payload, int size)
{
struct sk_buff *skb;
struct nlmsghdr *nlh;
int len = nlmsg_total_size(size);
- int flags = multi ? NLM_F_MULTI : 0;
- int t = done ? NLMSG_DONE : type;
skb = alloc_skb(len, GFP_ATOMIC);
if (!skb) {
@@ -2539,10 +2542,9 @@ iscsi_if_send_reply(uint32_t group, int seq, int type, int done, int multi,
return -ENOMEM;
}
- nlh = __nlmsg_put(skb, 0, 0, t, (len - sizeof(*nlh)), 0);
- nlh->nlmsg_flags = flags;
+ nlh = __nlmsg_put(skb, 0, 0, type, (len - sizeof(*nlh)), 0);
memcpy(nlmsg_data(nlh), payload, size);
- return iscsi_multicast_skb(skb, group, GFP_ATOMIC);
+ return iscsi_unicast_skb(skb, portid);
}
static int
@@ -3470,6 +3472,7 @@ static int
iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
{
int err = 0;
+ u32 portid;
struct iscsi_uevent *ev = nlmsg_data(nlh);
struct iscsi_transport *transport = NULL;
struct iscsi_internal *priv;
@@ -3490,10 +3493,12 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
if (!try_module_get(transport->owner))
return -EINVAL;
+ portid = NETLINK_CB(skb).portid;
+
switch (nlh->nlmsg_type) {
case ISCSI_UEVENT_CREATE_SESSION:
err = iscsi_if_create_session(priv, ep, ev,
- NETLINK_CB(skb).portid,
+ portid,
ev->u.c_session.initial_cmdsn,
ev->u.c_session.cmds_max,
ev->u.c_session.queue_depth);
@@ -3506,7 +3511,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
}
err = iscsi_if_create_session(priv, ep, ev,
- NETLINK_CB(skb).portid,
+ portid,
ev->u.c_bound_session.initial_cmdsn,
ev->u.c_bound_session.cmds_max,
ev->u.c_bound_session.queue_depth);
@@ -3664,6 +3669,8 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
static void
iscsi_if_rx(struct sk_buff *skb)
{
+ u32 portid = NETLINK_CB(skb).portid;
+
mutex_lock(&rx_queue_mutex);
while (skb->len >= NLMSG_HDRLEN) {
int err;
@@ -3699,8 +3706,8 @@ iscsi_if_rx(struct sk_buff *skb)
break;
if (ev->type == ISCSI_UEVENT_GET_CHAP && !err)
break;
- err = iscsi_if_send_reply(group, nlh->nlmsg_seq,
- nlh->nlmsg_type, 0, 0, ev, sizeof(*ev));
+ err = iscsi_if_send_reply(portid, nlh->nlmsg_type,
+ ev, sizeof(*ev));
} while (err < 0 && err != -ECONNREFUSED && err != -ESRCH);
skb_pull(skb, rlen);
}
--
2.14.3
^ permalink raw reply related
* Re: [RFC PATCH 2/3] netdev: kernel-only IFF_HIDDEN netdevice
From: Andrew Lunn @ 2018-04-09 22:15 UTC (permalink / raw)
To: Siwei Liu
Cc: David Miller, David Ahern, Jiri Pirko, si-wei liu,
Michael S. Tsirkin, Stephen Hemminger, Alexander Duyck,
Brandeburg, Jesse, Jakub Kicinski, Jason Wang, Samudrala, Sridhar,
Netdev, virtualization, virtio-dev
In-Reply-To: <CADGSJ211kA5ktADbwv2f4ma_ooBmYW4r7YwzGD2G_Gw5+wv5rw@mail.gmail.com>
> No, implementation wise I'd avoid changing the class on the fly. What
> I'm looking to is a means to add a secondary class or class aliasing
> mechanism for netdevs that allows mapping for a kernel device
> namespace (/class/net-kernel) to userspace (/class/net). Imagine
> creating symlinks between these two namespaces as an analogy. All
> userspace visible netdevs today will have both a kernel name and a
> userspace visible name, having one (/class/net) referecing the other
> (/class/net-kernel) in its own namespace. The newly introduced
> IFF_AUTO_MANAGED device will have a kernel name only
> (/class/net-kernel). As a result, the existing applications using
> /class/net don't break, while we're adding the kernel namespace that
> allows IFF_AUTO_MANAGED devices which will not be exposed to userspace
> at all.
My gut feeling is this whole scheme will not fly. You really should be
talking to GregKH.
Anyway, please remember that IFF_AUTO_MANAGED will need to be dynamic.
A device can start out as a normal device, and will change to being
automatic later, when the user on top of it probes.
Andrew
^ permalink raw reply
* Re: [RFC PATCH 2/3] netdev: kernel-only IFF_HIDDEN netdevice
From: Siwei Liu @ 2018-04-09 22:07 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, David Ahern, Jiri Pirko, si-wei liu,
Michael S. Tsirkin, Stephen Hemminger, Alexander Duyck,
Brandeburg, Jesse, Jakub Kicinski, Jason Wang, Samudrala, Sridhar,
Netdev, virtualization, virtio-dev
In-Reply-To: <20180407031919.GB11029@lunn.ch>
On Fri, Apr 6, 2018 at 8:19 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Siwei
>
>> I think everyone seems to agree not to fiddle with the ":" prefix, but
>> rather have a new class of network subsystem under /sys/class thus a
>> separate device namespace e.g. /sys/class/net-kernel for those
>> auto-managed lower netdevs is needed.
>
> How do you get a device into this new class? I don't know the Linux
> driver model too well, but to get a device out of one class and into
> another, i think you need to device_del(dev). modify dev->class and
> then device_add(dev). However, device_add() says you are not allowed
> to do this.
No, implementation wise I'd avoid changing the class on the fly. What
I'm looking to is a means to add a secondary class or class aliasing
mechanism for netdevs that allows mapping for a kernel device
namespace (/class/net-kernel) to userspace (/class/net). Imagine
creating symlinks between these two namespaces as an analogy. All
userspace visible netdevs today will have both a kernel name and a
userspace visible name, having one (/class/net) referecing the other
(/class/net-kernel) in its own namespace. The newly introduced
IFF_AUTO_MANAGED device will have a kernel name only
(/class/net-kernel). As a result, the existing applications using
/class/net don't break, while we're adding the kernel namespace that
allows IFF_AUTO_MANAGED devices which will not be exposed to userspace
at all.
As this requires changing the internals of driver model core it's a
rather big hammer approach I'd think. If there exists a better
implementation than this to allow adding a separate layer of in-kernel
device namespace, I'd more than welcome to hear about.
>
> And i don't even see how this helps. Are you also not going to call
> list_netdevice()? Are you going to add some other list for these
> devices in a different class?
list_netdevice() is still called. I think with the current RFC patch,
I've added two lists for netdevs under the kernel namespace:
dev_cmpl_list and name_cmpl_hlist. As a result of that, all userspace
netdevs get registered will be added to two types of lists: the
userspace list for e.g. dev_list, and also the kernelspace list e.g.
dev_cmpl_list (I can rename it to something more accurate). The
IFF_AUTO_MANAGED device will be only added to kernelspace list e.g.
dev_cmpl_list.
Hope all your questions are answered.
Thanks,
-Siwei
>
> Andrew
^ permalink raw reply
* Re: [RFC PATCH v2 00/14] Introducing AF_XDP support
From: William Tu @ 2018-04-09 21:51 UTC (permalink / raw)
To: Björn Töpel
Cc: Karlsson, Magnus, Alexander Duyck, Alexander Duyck,
John Fastabend, Alexei Starovoitov, Jesper Dangaard Brouer,
Willem de Bruijn, Daniel Borkmann,
Linux Kernel Network Developers, Björn Töpel,
michael.lundkvist, Brandeburg, Jesse, Anjali Singhai Jain,
qi.z.zhang, ravineet.singh
In-Reply-To: <20180327165919.17933-1-bjorn.topel@gmail.com>
On Tue, Mar 27, 2018 at 9:59 AM, Björn Töpel <bjorn.topel@gmail.com> wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> This RFC introduces a new address family called AF_XDP that is
> optimized for high performance packet processing and, in upcoming
> patch sets, zero-copy semantics. In this v2 version, we have removed
> all zero-copy related code in order to make it smaller, simpler and
> hopefully more review friendly. This RFC only supports copy-mode for
> the generic XDP path (XDP_SKB) for both RX and TX and copy-mode for RX
> using the XDP_DRV path. Zero-copy support requires XDP and driver
> changes that Jesper Dangaard Brouer is working on. Some of his work is
> already on the mailing list for review. We will publish our zero-copy
> support for RX and TX on top of his patch sets at a later point in
> time.
>
> An AF_XDP socket (XSK) is created with the normal socket()
> syscall. Associated with each XSK are two queues: the RX queue and the
> TX queue. A socket can receive packets on the RX queue and it can send
> packets on the TX queue. These queues are registered and sized with
> the setsockopts XDP_RX_QUEUE and XDP_TX_QUEUE, respectively. It is
> mandatory to have at least one of these queues for each socket. In
> contrast to AF_PACKET V2/V3 these descriptor queues are separated from
> packet buffers. An RX or TX descriptor points to a data buffer in a
> memory area called a UMEM. RX and TX can share the same UMEM so that a
> packet does not have to be copied between RX and TX. Moreover, if a
> packet needs to be kept for a while due to a possible retransmit, the
> descriptor that points to that packet can be changed to point to
> another and reused right away. This again avoids copying data.
>
> This new dedicated packet buffer area is called a UMEM. It consists of
> a number of equally size frames and each frame has a unique frame
> id. A descriptor in one of the queues references a frame by
> referencing its frame id. The user space allocates memory for this
> UMEM using whatever means it feels is most appropriate (malloc, mmap,
> huge pages, etc). This memory area is then registered with the kernel
> using the new setsockopt XDP_UMEM_REG. The UMEM also has two queues:
> the FILL queue and the COMPLETION queue. The fill queue is used by the
> application to send down frame ids for the kernel to fill in with RX
> packet data. References to these frames will then appear in the RX
> queue of the XSK once they have been received. The completion queue,
> on the other hand, contains frame ids that the kernel has transmitted
> completely and can now be used again by user space, for either TX or
> RX. Thus, the frame ids appearing in the completion queue are ids that
> were previously transmitted using the TX queue. In summary, the RX and
> FILL queues are used for the RX path and the TX and COMPLETION queues
> are used for the TX path.
>
Can we register a UMEM to multiple device's queue?
So far the l2fwd sample code is sending/receiving from the same
queue. I'm thinking about forwarding packets from one device to another.
Now I'm copying packets from one device's RX desc to another device's TX
completion queue. But this introduces one extra copy.
One way I can do is to call bpf_redirect helper function, but sometimes
I still need to process the packet in userspace.
I like this work!
Thanks a lot.
William
^ permalink raw reply
* Re: [net-next PATCH v3 01/11] soc: ti: K2G: enhancement to support QMSS in K2G NAVSS
From: Rob Herring @ 2018-04-09 21:12 UTC (permalink / raw)
To: Murali Karicheri
Cc: mark.rutland, ssantosh, malat, w-kwok2, devicetree, linux-kernel,
linux-arm-kernel, davem, netdev
In-Reply-To: <1522679881-25643-2-git-send-email-m-karicheri2@ti.com>
On Mon, Apr 02, 2018 at 10:37:51AM -0400, Murali Karicheri wrote:
> Navigator Subsystem (NAVSS) available on K2G SoC has a cut down
> version of QMSS with less number of queues, internal linking ram
> with lesser number of buffers etc. It doesn't have status and
> explicit push register space as in QMSS available on other K2 SoCs.
> So define reg indices specific to QMSS on K2G. This patch introduces
> "ti,66ak2g-navss-qm" compatibility to identify QMSS on K2G NAVSS
> and to customize the dts handling code. Per Device manual,
> descriptors with index less than or equal to regions0_size is in region 0
> in the case of K2 QMSS where as for QMSS on K2G, descriptors with index
> less than regions0_size is in region 0. So update the size accordingly in
> the regions0_size bits of the linking ram size 0 register.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> ---
> .../bindings/soc/ti/keystone-navigator-qmss.txt | 9 ++-
Reviewed-by: Rob Herring <robh@kernel.org>
> drivers/soc/ti/knav_qmss.h | 6 ++
> drivers/soc/ti/knav_qmss_queue.c | 90 ++++++++++++++++------
> 3 files changed, 82 insertions(+), 23 deletions(-)
^ permalink raw reply
* [PATCH] mISDN: Remove VLAs
From: Laura Abbott @ 2018-04-09 21:07 UTC (permalink / raw)
To: Karsten Keil, David S. Miller
Cc: Laura Abbott, Kees Cook, netdev, linux-kernel, kernel-hardening
There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. Remove the VLAs from the mISDN code by switching to using
kstrdup in one place and just using the upper bound in another.
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
drivers/isdn/mISDN/dsp_hwec.c | 8 +++++---
drivers/isdn/mISDN/l1oip_core.c | 2 +-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/isdn/mISDN/dsp_hwec.c b/drivers/isdn/mISDN/dsp_hwec.c
index a6e87076acc2..5336bbdbfdc5 100644
--- a/drivers/isdn/mISDN/dsp_hwec.c
+++ b/drivers/isdn/mISDN/dsp_hwec.c
@@ -68,12 +68,12 @@ void dsp_hwec_enable(struct dsp *dsp, const char *arg)
goto _do;
{
- char _dup[len + 1];
char *dup, *tok, *name, *val;
int tmp;
- strcpy(_dup, arg);
- dup = _dup;
+ dup = kstrdup(arg, GFP_ATOMIC);
+ if (!dup)
+ return;
while ((tok = strsep(&dup, ","))) {
if (!strlen(tok))
@@ -89,6 +89,8 @@ void dsp_hwec_enable(struct dsp *dsp, const char *arg)
deftaps = tmp;
}
}
+
+ kfree(dup);
}
_do:
diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 21d50e4cc5e1..df594245deca 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -279,7 +279,7 @@ l1oip_socket_send(struct l1oip *hc, u8 localcodec, u8 channel, u32 chanmask,
u16 timebase, u8 *buf, int len)
{
u8 *p;
- u8 frame[len + 32];
+ u8 frame[L1OIP_MAX_PERFRAME + 32];
struct socket *socket = NULL;
if (debug & DEBUG_L1OIP_MSG)
--
2.14.3
^ permalink raw reply related
* Re: [PATCH AUTOSEL for 4.9 160/293] MIPS: Give __secure_computing() access to syscall arguments.
From: James Hogan @ 2018-04-09 20:26 UTC (permalink / raw)
To: Sasha Levin
Cc: stable@vger.kernel.org, linux-kernel@vger.kernel.org, David Daney,
Alexei Starovoitov, Daniel Borkmann, Matt Redfearn,
netdev@vger.kernel.org, linux-mips@linux-mips.org, Ralf Baechle
In-Reply-To: <20180409002239.163177-160-alexander.levin@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 581 bytes --]
On Mon, Apr 09, 2018 at 12:24:58AM +0000, Sasha Levin wrote:
> From: David Daney <david.daney@cavium.com>
>
> [ Upstream commit 669c4092225f0ed5df12ebee654581b558a5e3ed ]
>
> KProbes of __seccomp_filter() are not very useful without access to
> the syscall arguments.
>
> Do what x86 does, and populate a struct seccomp_data to be passed to
> __secure_computing(). This allows samples/bpf/tracex5 to extract a
> sensible trace.
This broke o32 indirect syscalls, and was fixed by commit 3d729deaf287
("MIPS: seccomp: Fix indirect syscall args").
Cheers
James
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Patch "x86/asm: Don't use RBP as a temporary register in csum_partial_copy_generic()" has been added to the 4.9-stable tree
From: gregkh @ 2018-04-09 20:00 UTC (permalink / raw)
To: jpoimboe, alexander.levin, andreyknvl, davem, dvyukov, edumazet,
gregkh, kcc, marcelo.leitner, mingo, netdev, nhorman, peterz,
syzkaller, tglx, torvalds, vyasevich, xiyou.wangcong
Cc: stable, stable-commits
This is a note to let you know that I've just added the patch titled
x86/asm: Don't use RBP as a temporary register in csum_partial_copy_generic()
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
x86-asm-don-t-use-rbp-as-a-temporary-register-in-csum_partial_copy_generic.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
>From foo@baz Mon Apr 9 17:09:24 CEST 2018
From: Josh Poimboeuf <jpoimboe@redhat.com>
Date: Thu, 4 May 2017 09:51:40 -0500
Subject: x86/asm: Don't use RBP as a temporary register in csum_partial_copy_generic()
From: Josh Poimboeuf <jpoimboe@redhat.com>
[ Upstream commit 42fc6c6cb1662ba2fa727dd01c9473c63be4e3b6 ]
Andrey Konovalov reported the following warning while fuzzing the kernel
with syzkaller:
WARNING: kernel stack regs at ffff8800686869f8 in a.out:4933 has bad 'bp' value c3fc855a10167ec0
The unwinder dump revealed that RBP had a bad value when an interrupt
occurred in csum_partial_copy_generic().
That function saves RBP on the stack and then overwrites it, using it as
a scratch register. That's problematic because it breaks stack traces
if an interrupt occurs in the middle of the function.
Replace the usage of RBP with another callee-saved register (R15) so
stack traces are no longer affected.
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Tested-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: David S . Miller <davem@davemloft.net>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: linux-sctp@vger.kernel.org
Cc: netdev <netdev@vger.kernel.org>
Cc: syzkaller <syzkaller@googlegroups.com>
Link: http://lkml.kernel.org/r/4b03a961efda5ec9bfe46b7b9c9ad72d1efad343.1493909486.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/x86/lib/csum-copy_64.S | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--- a/arch/x86/lib/csum-copy_64.S
+++ b/arch/x86/lib/csum-copy_64.S
@@ -55,7 +55,7 @@ ENTRY(csum_partial_copy_generic)
movq %r12, 3*8(%rsp)
movq %r14, 4*8(%rsp)
movq %r13, 5*8(%rsp)
- movq %rbp, 6*8(%rsp)
+ movq %r15, 6*8(%rsp)
movq %r8, (%rsp)
movq %r9, 1*8(%rsp)
@@ -74,7 +74,7 @@ ENTRY(csum_partial_copy_generic)
/* main loop. clear in 64 byte blocks */
/* r9: zero, r8: temp2, rbx: temp1, rax: sum, rcx: saved length */
/* r11: temp3, rdx: temp4, r12 loopcnt */
- /* r10: temp5, rbp: temp6, r14 temp7, r13 temp8 */
+ /* r10: temp5, r15: temp6, r14 temp7, r13 temp8 */
.p2align 4
.Lloop:
source
@@ -89,7 +89,7 @@ ENTRY(csum_partial_copy_generic)
source
movq 32(%rdi), %r10
source
- movq 40(%rdi), %rbp
+ movq 40(%rdi), %r15
source
movq 48(%rdi), %r14
source
@@ -103,7 +103,7 @@ ENTRY(csum_partial_copy_generic)
adcq %r11, %rax
adcq %rdx, %rax
adcq %r10, %rax
- adcq %rbp, %rax
+ adcq %r15, %rax
adcq %r14, %rax
adcq %r13, %rax
@@ -121,7 +121,7 @@ ENTRY(csum_partial_copy_generic)
dest
movq %r10, 32(%rsi)
dest
- movq %rbp, 40(%rsi)
+ movq %r15, 40(%rsi)
dest
movq %r14, 48(%rsi)
dest
@@ -203,7 +203,7 @@ ENTRY(csum_partial_copy_generic)
movq 3*8(%rsp), %r12
movq 4*8(%rsp), %r14
movq 5*8(%rsp), %r13
- movq 6*8(%rsp), %rbp
+ movq 6*8(%rsp), %r15
addq $7*8, %rsp
ret
Patches currently in stable-queue which might be from jpoimboe@redhat.com are
queue-4.9/x86-asm-don-t-use-rbp-as-a-temporary-register-in-csum_partial_copy_generic.patch
^ permalink raw reply
* RFC: ti_hecc: don't repoll but interrupt again
From: Jeroen Hofstee @ 2018-04-09 19:49 UTC (permalink / raw)
To: netdev
Hello,
I posted below RFC to the linux-can ML, but it might be actually
more appropriate for netdev (since it involves napi). This driver
is lying a bit about the amount of work done. Changing that (always
look at work done instead of the hw state) seems to solve the issue,
but I have no clear understanding why that is (and why it takes down
the ethernet stack as well).
If someone has suggestions / ideas on what to check, please let me
know.
Regards,
Jeroen
The mail send to linux-can:
While updating to Linux 4.9.93, the ti_hecc causes several problems. One
of them is that under high load on the CAN-bus and ethernet, the ethernet
connection completely stalls and won't recover. The patch below seems to
fix that (as in read as much as we can and always re-enable interrupts,
it will poll again thereafter).
It would be appreciated if someone more familiar with this code can
comment on it.
Thanks in advance,
Jeroen
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -625,20 +625,16 @@ static int ti_hecc_rx_poll(struct napi_struct
*napi, int quota)
spin_unlock_irqrestore(&priv->mbx_lock, flags);
} else if (priv->rx_next == HECC_MAX_TX_MBOX - 1) {
priv->rx_next = HECC_RX_FIRST_MBOX;
- break;
}
}
/* Enable packet interrupt if all pkts are handled */
- if (hecc_read(priv, HECC_CANRMP) == 0) {
+ if (num_pkts < quota) {
napi_complete(napi);
/* Re-enable RX mailbox interrupts */
mbx_mask = hecc_read(priv, HECC_CANMIM);
mbx_mask |= HECC_TX_MBOX_MASK;
hecc_write(priv, HECC_CANMIM, mbx_mask);
- } else {
- /* repoll is done only if whole budget is used */
- num_pkts = quota;
}
^ permalink raw reply
* [PATCH RESEND net] vhost: fix vhost_vq_access_ok() log check
From: Michael S. Tsirkin @ 2018-04-09 19:40 UTC (permalink / raw)
To: David Miller, netdev, virtualization
Cc: linux-kernel, jasowang, kvm, mst, syzkaller-bugs, Stefan Hajnoczi
In-Reply-To: <20180409131021.5132-1-stefanha@redhat.com>
From: Stefan Hajnoczi <stefanha@redhat.com>
Commit d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log
when IOTLB is enabled") introduced a regression. The logic was
originally:
if (vq->iotlb)
return 1;
return A && B;
After the patch the short-circuit logic for A was inverted:
if (A || vq->iotlb)
return A;
return B;
The correct logic is:
if (!A || vq->iotlb)
return A;
return B;
Reported-by: syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5320039671b7..f6af4210679a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1246,7 +1246,7 @@ int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
int ret = vq_log_access_ok(vq, vq->log_base);
- if (ret || vq->iotlb)
+ if (!ret || vq->iotlb)
return ret;
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
--
2.14.3
^ permalink raw reply related
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Samudrala, Sridhar @ 2018-04-09 18:47 UTC (permalink / raw)
To: Jiri Pirko
Cc: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, jasowang,
loseweigh
In-Reply-To: <20180409080751.GE19345@nanopsycho>
On 4/9/2018 1:07 AM, Jiri Pirko wrote:
> Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>> On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>>> Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
> [...]
>
>>>> +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>>>> + struct net_device *child_netdev)
>>>> +{
>>>> + struct virtnet_bypass_info *vbi;
>>>> + bool backup;
>>>> +
>>>> + vbi = netdev_priv(bypass_netdev);
>>>> + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>>>> + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>>>> + rtnl_dereference(vbi->active_netdev)) {
>>>> + netdev_info(bypass_netdev,
>>>> + "%s attempting to join bypass dev when %s already present\n",
>>>> + child_netdev->name, backup ? "backup" : "active");
>>> Bypass module should check if there is already some other netdev
>>> enslaved and refuse right there.
>> This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>> as its bypass_netdev is same as the backup_netdev.
>> Will add a flag while registering with the bypass module to indicate if the driver is doing
>> a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>> for 3 netdev scenario.
> Just let me undestand it clearly. What I expect the difference would be
> between 2netdev and3 netdev model is this:
> 2netdev:
> bypass_master
> /
> /
> VF_slave
>
> 3netdev:
> bypass_master
> / \
> / \
> VF_slave backup_slave
>
> Is that correct? If not, how does it look like?
>
>
Looks correct.
VF_slave and backup_slave are the original netdevs and are present in both the models.
In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
marked as the 2 slaves of this new netdev.
In the 2 netdev model, backup_slave acts as bypass_master and the bypass module doesn't
have access to netdev_priv of the backup_slave.
Once i moved all the ndo_ops of the master netdev to bypass module, i realized that we can
move the create/destroy of the upper netdev also to bypass.c.
That way the changes to virtio_net become very minimal.
With these updates, bypass module now supports both the models by exporting 2 sets of
functions.
3 netdev:
int bypass_master_create(struct net_device *backup_netdev,
struct bypass_master **pbypass_master);
void bypass_master_destroy(struct bypass_master *bypass_master);
2 netdev:
int bypass_master_register(struct net_device *backup_netdev, struct bypass_ops *ops,
struct bypass_master **pbypass_master);
void bypass_master_unregister(struct bypass_master *bypass_master);
Will send the next revision in a day or two.
Thanks
Sridhar
^ permalink raw reply
* Re: [PATCH bpf] bpf/tracing: fix a deadlock in perf_event_detach_bpf_prog
From: Yonghong Song @ 2018-04-09 18:41 UTC (permalink / raw)
To: Alexei Starovoitov, daniel, netdev; +Cc: kernel-team
In-Reply-To: <88effd87-1356-38b4-96e5-b90ab869b2a8@fb.com>
On 4/9/18 9:47 AM, Alexei Starovoitov wrote:
> On 4/9/18 9:18 AM, Yonghong Song wrote:
>> syzbot reported a possible deadlock in perf_event_detach_bpf_prog.
> ...
>> @@ -985,16 +986,31 @@ int perf_event_query_prog_array(struct
>> perf_event *event, void __user *info)
>> return -EINVAL;
>> if (copy_from_user(&query, uquery, sizeof(query)))
>> return -EFAULT;
>> - if (query.ids_len > BPF_TRACE_MAX_PROGS)
>> +
>> + ids_len = query.ids_len;
>> + if (ids_len > BPF_TRACE_MAX_PROGS)
>> return -E2BIG;
>> + ids = kcalloc(ids_len, sizeof(u32), GFP_USER | __GFP_NOWARN);
>> + if (!ids)
>> + return -ENOMEM;
>>
>> mutex_lock(&bpf_event_mutex);
>> ret = bpf_prog_array_copy_info(event->tp_event->prog_array,
>> - uquery->ids,
>> - query.ids_len,
>> - &uquery->prog_cnt);
>> + ids,
>> + ids_len,
>> + &prog_cnt);
>> mutex_unlock(&bpf_event_mutex);
>>
>> + if (!ret || ret == -ENOSPC) {
>> + if (copy_to_user(&uquery->prog_cnt, &prog_cnt,
>> sizeof(prog_cnt)) ||
>> + copy_to_user(uquery->ids, ids, ids_len * sizeof(u32))) {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> + }
>> +
>> +out:
>> + kfree(ids);
>
> alloc/free just to avoid this locking dependency feels suboptimal.
We actually already did kcalloc/kfree in bpf_prog_array_copy_to_user.
In that function, we did not copy_to_user one id at a time.
We allocate a temporary array and store the result there
and at the end, we call one copy_to_user to copy to the user buffer.
The patch here just moved this allocation and associated copy_to_user
out of the function and bpf_event_mutex. It did not introduce new
allocations.
>
> may be we can get rid of bpf_event_mutex in some cases?
> the perf event itself is locked via perf_event_ctx_lock() when we're
> calling perf_event_query_prog_array, perf_event_attach|detach_bpf_prog.
> I forgot what was the motivation for us to introduce it in the
> first place.
The original motivation for the lock to make sure bpf_prog_array
does not change during middle of attach/detach/query. it looks like
we have:
. perf_event_attach|query under perf_event_ctx_lock
. perf_event_detach not under perf_event_ctx_lock
Introducing perf_event_ctx_lock to perf_event_detach could still
have the deadlock.
>
^ permalink raw reply
* Re: [PATCH iproute2-next 1/1] tc: jsonify skbedit action
From: Roman Mashak @ 2018-04-09 17:57 UTC (permalink / raw)
To: David Ahern; +Cc: stephen, netdev, kernel, jhs, xiyou.wangcong, jiri
In-Reply-To: <f38705ad-38d0-9e65-510c-c71095e5afd8@gmail.com>
David Ahern <dsahern@gmail.com> writes:
> On 4/3/18 1:24 PM, Roman Mashak wrote:
>> if (tb[TCA_SKBEDIT_PTYPE] != NULL) {
>> - ptype = RTA_DATA(tb[TCA_SKBEDIT_PTYPE]);
>> - if (*ptype == PACKET_HOST)
>> - fprintf(f, " ptype host");
>> - else if (*ptype == PACKET_BROADCAST)
>> - fprintf(f, " ptype broadcast");
>> - else if (*ptype == PACKET_MULTICAST)
>> - fprintf(f, " ptype multicast");
>> - else if (*ptype == PACKET_OTHERHOST)
>> - fprintf(f, " ptype otherhost");
>> + ptype = rta_getattr_u16(tb[TCA_SKBEDIT_PTYPE]);
>> + if (ptype == PACKET_HOST)
>> + print_string(PRINT_ANY, "ptype", " %s", "ptype host");
>> + else if (ptype == PACKET_BROADCAST)
>> + print_string(PRINT_ANY, "ptype", " %s",
>> + "ptype broadcast");
>> + else if (ptype == PACKET_MULTICAST)
>> + print_string(PRINT_ANY, "ptype", " %s",
>> + "ptype multicast");
>> + else if (ptype == PACKET_OTHERHOST)
>> + print_string(PRINT_ANY, "ptype", " %s",
>> + "ptype otherhost");
>
> Shouldn't that be:
> print_string(PRINT_ANY, "ptype", "ptype %s", "otherhost");
>
> And ditto for the other strings.
>
>> else
>> - fprintf(f, " ptype %d", *ptype);
>> + print_uint(PRINT_ANY, "ptype", " %u", ptype);
>
> And then this one needs 'ptype' before %u
OK. I will send v2.
^ permalink raw reply
* RE: [Intel-wired-lan] [next-queue PATCH v6 10/10] igb: Add support for adding offloaded clsflower filters
From: Vinicius Costa Gomes @ 2018-04-09 17:36 UTC (permalink / raw)
To: Brown, Aaron F, intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Sanchez-Palencia, Jesus
In-Reply-To: <309B89C4C689E141A5FF6A0C5FB2118B8C82598F@ORSMSX101.amr.corp.intel.com>
Hi,
"Brown, Aaron F" <aaron.f.brown@intel.com> writes:
>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
>> Behalf Of Vinicius Costa Gomes
>> Sent: Thursday, March 29, 2018 2:08 PM
>> To: intel-wired-lan@lists.osuosl.org
>> Cc: netdev@vger.kernel.org; Sanchez-Palencia, Jesus <jesus.sanchez-
>> palencia@intel.com>
>> Subject: [Intel-wired-lan] [next-queue PATCH v6 10/10] igb: Add support for
>> adding offloaded clsflower filters
>>
>> This allows filters added by tc-flower and specifying MAC addresses,
>> Ethernet types, and the VLAN priority field, to be offloaded to the
>> controller.
>
> Can I get a brief explanation for enabling this? I'm currently happy
> with this patch series from a regression perspective, but am
> personally a bit, umm, challenged with tc in general but would like to
> run it through the paces a bit. If it can be done in a one or two
> liner I think it would be a good addition to the patch description.
Of course, my bad for not adding those examples since the begining.
Cheers,
^ permalink raw reply
* RE: [Intel-wired-lan] [next-queue PATCH v6 08/10] igb: Add MAC address support for ethtool nftuple filters
From: Vinicius Costa Gomes @ 2018-04-09 17:33 UTC (permalink / raw)
To: Brown, Aaron F, intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Sanchez-Palencia, Jesus
In-Reply-To: <309B89C4C689E141A5FF6A0C5FB2118B8C825976@ORSMSX101.amr.corp.intel.com>
Hi,
"Brown, Aaron F" <aaron.f.brown@intel.com> writes:
[...]
>
>>
>> I added that note in the hope that someone else would have an stronger
>> opinion about what to do.
>
> I don't have a strong opinion beyond my preference for an ideal world
> where everything works :) If the part simply cannot filter on the src
> address as a whole without the protocol I would ideally prefer an
> attempt in ethtool to set the filter on src address as a whole to
> return an error WHILE still allowing the filter to be set on an
> ethertype when the proto keyword is issued. If ethtool does not allow
> that fine grain of control then I think the way it is now is good, I'd
> rather have the annoyance of being able to set a filter that does
> nothing then not be able to set the more specific filter at all.
We are agreed. The next version of this series implements just that:
specifying the src address alone is an error, but if the classifier has
a src address and any other filter, it works.
Cheers,
^ permalink raw reply
* [PATCH RFC iptables] iptables: Per-net ns lock
From: Kirill Tkhai @ 2018-04-09 16:55 UTC (permalink / raw)
To: netdev, pablo, rstoyanov1, ptikhomirov, rstoyanov1, avagin,
ktkhai
In CRIU and LXC-restore we met the situation,
when iptables in container can't be restored
because of permission denied:
https://github.com/checkpoint-restore/criu/issues/469
Containers want to restore their own net ns,
while they may have no their own mnt ns.
This case they share host's /run/xtables.lock
file, but they may not have permission to open
it.
Patch makes /run/xtables.lock to be per-namespace,
i.e., to refer to the caller task's net ns.
What you think?
Thanks,
Kirill
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
iptables/xshared.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 06db72d4..b6dbe4e7 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -254,7 +254,12 @@ static int xtables_lock(int wait, struct timeval *wait_interval)
time_left.tv_sec = wait;
time_left.tv_usec = 0;
- fd = open(XT_LOCK_NAME, O_CREAT, 0600);
+ if (symlink("/proc/self/ns/net", XT_LOCK_NAME) != 0 &&
+ errno != EEXIST) {
+ fprintf(stderr, "Fatal: can't create lock file\n");
+ return XT_LOCK_FAILED;
+ }
+ fd = open(XT_LOCK_NAME, O_RDONLY);
if (fd < 0) {
fprintf(stderr, "Fatal: can't open lock file %s: %s\n",
XT_LOCK_NAME, strerror(errno));
^ permalink raw reply related
* Re: [RFC PATCH bpf-next 2/6] bpf: add bpf_get_stack helper
From: Yonghong Song @ 2018-04-09 16:52 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, netdev; +Cc: kernel-team
In-Reply-To: <181ba790-db7c-943c-8b77-aabe10f1a1de@iogearbox.net>
On 4/9/18 3:01 AM, Daniel Borkmann wrote:
> On 04/09/2018 07:02 AM, Alexei Starovoitov wrote:
>> On 4/8/18 9:53 PM, Yonghong Song wrote:
>>>>> @@ -1004,7 +1007,8 @@ static void __bpf_prog_put(struct bpf_prog
>>>>> *prog, bool do_idr_lock)
>>>>> bpf_prog_kallsyms_del(prog->aux->func[i]);
>>>>> bpf_prog_kallsyms_del(prog);
>>>>>
>>>>> - call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
>>>>> + synchronize_rcu();
>>>>> + __bpf_prog_put_rcu(&prog->aux->rcu);
>>>>
>>>> there should have been lockdep splat.
>>>> We cannot call synchronize_rcu here, since we cannot sleep
>>>> in some cases.
>>>
>>> Let me double check this. The following is the reason
>>> why I am using synchronize_rcu().
>>>
>>> With call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu) and
>>> _bpf_prog_put_rcu calls put_callchain_buffers() which
>>> calls mutex_lock(), the runtime with CONFIG_DEBUG_ATOMIC_SLEEP=y
>>> will complains since potential sleep inside the call_rcu is not
>>> allowed.
>>
>> I see. Indeed. We cannot call put_callchain_buffers() from rcu callback,
>> but doing synchronize_rcu() here is also not possible.
>> How about moving put_callchain into bpf_prog_free_deferred() ?
>
> +1, the assumption is that you can call bpf_prog_put() and also the
> bpf_map_put() from any context. Sleeping here for a long time might
> subtly break things badly.
Thanks for the suggestion! This should work.
^ permalink raw reply
* Re: [PATCH bpf] bpf/tracing: fix a deadlock in perf_event_detach_bpf_prog
From: Alexei Starovoitov @ 2018-04-09 16:47 UTC (permalink / raw)
To: Yonghong Song, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20180409161819.4024793-1-yhs@fb.com>
On 4/9/18 9:18 AM, Yonghong Song wrote:
> syzbot reported a possible deadlock in perf_event_detach_bpf_prog.
...
> @@ -985,16 +986,31 @@ int perf_event_query_prog_array(struct perf_event *event, void __user *info)
> return -EINVAL;
> if (copy_from_user(&query, uquery, sizeof(query)))
> return -EFAULT;
> - if (query.ids_len > BPF_TRACE_MAX_PROGS)
> +
> + ids_len = query.ids_len;
> + if (ids_len > BPF_TRACE_MAX_PROGS)
> return -E2BIG;
> + ids = kcalloc(ids_len, sizeof(u32), GFP_USER | __GFP_NOWARN);
> + if (!ids)
> + return -ENOMEM;
>
> mutex_lock(&bpf_event_mutex);
> ret = bpf_prog_array_copy_info(event->tp_event->prog_array,
> - uquery->ids,
> - query.ids_len,
> - &uquery->prog_cnt);
> + ids,
> + ids_len,
> + &prog_cnt);
> mutex_unlock(&bpf_event_mutex);
>
> + if (!ret || ret == -ENOSPC) {
> + if (copy_to_user(&uquery->prog_cnt, &prog_cnt, sizeof(prog_cnt)) ||
> + copy_to_user(uquery->ids, ids, ids_len * sizeof(u32))) {
> + ret = -EFAULT;
> + goto out;
> + }
> + }
> +
> +out:
> + kfree(ids);
alloc/free just to avoid this locking dependency feels suboptimal.
may be we can get rid of bpf_event_mutex in some cases?
the perf event itself is locked via perf_event_ctx_lock() when we're
calling perf_event_query_prog_array, perf_event_attach|detach_bpf_prog.
I forgot what was the motivation for us to introduce it in the
first place.
^ permalink raw reply
* Re: [RFC v3 net-next 13/18] net/sched: Introduce the TBS Qdisc
From: Jesus Sanchez-Palencia @ 2018-04-09 16:36 UTC (permalink / raw)
To: Thomas Gleixner
Cc: netdev, jhs, xiyou.wangcong, jiri, vinicius.gomes, richardcochran,
anna-maria, henrik, John Stultz, levi.pearson, edumazet, willemb,
mlichvar
In-Reply-To: <alpine.DEB.2.21.1803280808490.3247@nanos.tec.linutronix.de>
Hi Thomas,
On 03/28/2018 12:48 AM, Thomas Gleixner wrote:
(...)
>
> There are two modes:
>
> 1) Send at the given TX time (Explicit mode)
>
> 2) Send before given TX time (Deadline mode)
>
> There is no need to specify 'drop if late' simply because if the message is
> handed in past the given TX time, it's too late by definition. What you are
> trying to implement is a hybrid of TSN and general purpose (not time aware)
> networking in one go. And you do that because your overall design is not
> looking at the big picture. You designed from a given use case assumption
> and tried to fit other things into it with duct tape.
Ok, I see the difference now, thanks. I have just two more questions about the
deadline mode, please see below.
(...)
>
>>> Coming back to the overall scheme. If you start upfront with a time slice
>>> manager which is designed to:
>>>
>>> - Handle multiple channels
>>>
>>> - Expose the time constraints, properties per channel
>>>
>>> then you can fit all kind of use cases, whether designed by committee or
>>> not. You can configure that thing per node or network wide. It does not
>>> make a difference. The only difference are the resulting constraints.
>>
>>
>> Ok, and I believe the above was covered by what we had proposed before, unless
>> what you meant by time constraints is beyond the configured port schedule.
>>
>> Are you suggesting that we'll need to have a kernel entity that is not only
>> aware of the current traffic classes 'schedule', but also of the resources that
>> are still available for new streams to be accommodated into the classes? Putting
>> it differently, is the TAS you envision just an entity that runs a schedule, or
>> is it a time-aware 'orchestrator'?
>
> In the first place its something which runs a defined schedule.
>
> The accomodation for new streams is required, but not necessarily at the
> root qdisc level. That might be a qdisc feeding into it.
>
> Assume you have a bandwidth reservation, aka time slot, for audio. If your
> audio related qdisc does deadline scheduling then you can add new streams
> to it up to the point where it's not longer able to fit.
>
> The only thing which might be needed at the root qdisc is the ability to
> utilize unused time slots for other purposes, but that's not required to be
> there in the first place as long as its designed in a way that it can be
> added later on.
Ok, agreed.
>
>>> So lets look once more at the picture in an abstract way:
>>>
>>> [ NIC ]
>>> |
>>> [ Time slice manager ]
>>> | |
>>> [ Ch 0 ] ... [ Ch N ]
>>>
>>> So you have a bunch of properties here:
>>>
>>> 1) Number of Channels ranging from 1 to N
>>>
>>> 2) Start point, slice period and slice length per channel
>>
>> Ok, so we agree that a TAS entity is needed. Assuming that channels are traffic
>> classes, do you have something else in mind other than a new root qdisc?
>
> Whatever you call it, the important point is that it is the gate keeper to
> the network adapter and there is no way around it. It fully controls the
> timed schedule how simple or how complex it may be.
Ok, and I've finally understood the nuance between the above and what we had
planned initially.
(...)
>>
>> * TAS:
>>
>> The idea we are currently exploring is to add a "time-aware", priority based
>> qdisc, that also exposes the Tx queues available and provides a mechanism for
>> mapping priority <-> traffic class <-> Tx queues in a similar fashion as
>> mqprio. We are calling this qdisc 'taprio', and its 'tc' cmd line would be:
>>
>> $ $ tc qdisc add dev ens4 parent root handle 100 taprio num_tc 4 \
>> map 2 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
>> queues 0 1 2 3 \
>> sched-file gates.sched [base-time <interval>] \
>> [cycle-time <interval>] [extension-time <interval>]
>>
>> <file> is multi-line, with each line being of the following format:
>> <cmd> <gate mask> <interval in nanoseconds>
>>
>> Qbv only defines one <cmd>: "S" for 'SetGates'
>>
>> For example:
>>
>> S 0x01 300
>> S 0x03 500
>>
>> This means that there are two intervals, the first will have the gate
>> for traffic class 0 open for 300 nanoseconds, the second will have
>> both traffic classes open for 500 nanoseconds.
>
> To accomodate stuff like control systems you also need a base line, which
> is not expressed as interval. Otherwise you can't schedule network wide
> explicit plans. That's either an absolute network-time (TAI) time stamp or
> an offset to a well defined network-time (TAI) time stamp, e.g. start of
> epoch or something else which is agreed on. The actual schedule then fast
> forwards past now (TAI) and sets up the slots from there. That makes node
> hotplug possible as well.
Sure, and the [base-time <interval>] on the command line above was actually
wrong. It should have been expressed as [base-time <timestamp>].
>> It would handle multiple channels and expose their constraints / properties.
>> Each channel also becomes a traffic class, so other qdiscs can be attached to
>> them separately.
>
> Right.
>
>> So, in summary, because our entire design is based on qdisc interfaces, what we
>> had proposed was a root qdisc (the time slice manager, as you put) that allows
>> for other qdiscs to be attached to each channel. The inner qdiscs define the
>> queueing modes for each channel, and tbs is just one of those modes. I
>> understand now that you want to allow for fully dynamic use-cases to be
>> supported as well, which we hadn't covered with our TAS proposal before because
>> we hadn't envisioned it being used for these systems' design.
>
> Yes, you have the root qdisc, which is in charge of the overall scheduling
> plan, how complex or not it is defined does not matter. It exposes traffic
> classes which have properties defined by the configuration.
Perfect. Let's see if we can agree on an overall plan, then. Hopefully I'm not
missing anything.
For the above we'll develop a new qdisc, designed along the 'taprio' ideas, thus
a Qbv style scheduler, to be used as root qdisc. It can run the schedule inside
the kernel or just offload it to the NIC if supported. Similarly to the other
multiqueue qdiscs, it will expose the HW Tx queues.
What is new here from the ideas we shared last year is that this new root qdisc
will be responsible for calling the attached qdiscs' dequeue functions during
their timeslices, making it the only entity capable of enqueueing packets into
the NIC.
This is the "global scheduler", but we still need the txtime aware qdisc. For
that, we'll modify tbs to accommodate the feedback from this thread. More below.
>
> The qdiscs which are attached to those traffic classes can be anything
> including:
>
> - Simple feed through (Applications are time contraints aware and set the
> exact schedule). qdisc has admission control.
This will be provided by the tbs qdisc. It will still provide a txtime sorted
list and hw offload, but now there will be a per-socket option that tells the
qdisc if the per-packet timestamp is the txtime (i.e. explicit mode, as you've
called it) or a deadline. The drop_if_late flag will be removed.
When in explicit mode, packets from that socket are dequeued from the qdisc
during its time slice if their [(txtime - delta) < now].
>
> - Deadline aware qdisc to handle e.g. A/V streams. Applications are aware
> of time constraints and provide the packet deadline. qdisc has admission
> control. This can be a simple first comes, first served scheduler or
> something like EDF which allows optimized utilization. The qdisc sets
> the TX time depending on the deadline and feeds into the root.
This will be provided by tbs if the socket which is transmitting packets is
configured for deadline mode.
For the deadline -> txtime conversion, what I have in mind is: when dequeue is
called tbs will just change the skbuff's timestamp from the deadline to 'now'
(i.e. as soon as possible) and dequeue the packet. Would that be enough or
should we use the delta parameter of the qdisc on this case add make [txtime =
now + delta]? The only benefit of doing so would be to provide a configurable
'fudge' factor.
Another question for this mode (but perhaps that applies to both modes) is, what
if the qdisc misses the deadline for *any* reason? I'm assuming it should drop
the packet during dequeue.
Putting it all together, we end up with:
1) a new txtime aware qdisc, tbs, to be used per queue. Its cli will look like:
$ tc qdisc add (...) tbs clockid CLOCK_REALTIME delta 150000 offload sorting
2) a new cmsg-interface for setting a per-packet timestamp that will be used
either as a txtime or as deadline by tbs (and further the NIC driver for the
offlaod case): SCM_TXTIME.
3) a new socket option: SO_TXTIME. It will be used to enable the feature for a
socket, and will have as parameters a clockid and a txtime mode (deadline or
explicit), that defines the semantics of the timestamp set on packets using
SCM_TXTIME.
4) a new #define DYNAMIC_CLOCKID 15 added to include/uapi/linux/time.h .
5) a new schedule-aware qdisc, 'tas' or 'taprio', to be used per port. Its cli
will look like what was proposed for taprio (base time being an absolute timestamp).
If we all agree with the above, we will start by closing on 1-4 asap and will
focus on 5 next.
How does that sound?
Thanks,
Jesus
>
> - FIFO/PRIO/XXX for general traffic. Applications do not know anything
> about timing constraints. These qdiscs obviously have neither admission
> control nor do they set a TX time. The root qdisc just pulls from there
> when the assigned time slot is due or if it (optionally) decides to use
> underutilized time slots from other classes.
>
> - .... Add your favourite scheduling mode(s).
>
> Thanks,
>
> tglx
>
^ permalink raw reply
* [PATCH bpf] bpf/tracing: fix a deadlock in perf_event_detach_bpf_prog
From: Yonghong Song @ 2018-04-09 16:18 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: kernel-team
syzbot reported a possible deadlock in perf_event_detach_bpf_prog.
The error details:
======================================================
WARNING: possible circular locking dependency detected
4.16.0-rc7+ #3 Not tainted
------------------------------------------------------
syz-executor7/24531 is trying to acquire lock:
(bpf_event_mutex){+.+.}, at: [<000000008a849b07>] perf_event_detach_bpf_prog+0x92/0x3d0 kernel/trace/bpf_trace.c:854
but task is already holding lock:
(&mm->mmap_sem){++++}, at: [<0000000038768f87>] vm_mmap_pgoff+0x198/0x280 mm/util.c:353
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&mm->mmap_sem){++++}:
__might_fault+0x13a/0x1d0 mm/memory.c:4571
_copy_to_user+0x2c/0xc0 lib/usercopy.c:25
copy_to_user include/linux/uaccess.h:155 [inline]
bpf_prog_array_copy_info+0xf2/0x1c0 kernel/bpf/core.c:1694
perf_event_query_prog_array+0x1c7/0x2c0 kernel/trace/bpf_trace.c:891
_perf_ioctl kernel/events/core.c:4750 [inline]
perf_ioctl+0x3e1/0x1480 kernel/events/core.c:4770
vfs_ioctl fs/ioctl.c:46 [inline]
do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
SYSC_ioctl fs/ioctl.c:701 [inline]
SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x42/0xb7
-> #0 (bpf_event_mutex){+.+.}:
lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:3920
__mutex_lock_common kernel/locking/mutex.c:756 [inline]
__mutex_lock+0x16f/0x1a80 kernel/locking/mutex.c:893
mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:908
perf_event_detach_bpf_prog+0x92/0x3d0 kernel/trace/bpf_trace.c:854
perf_event_free_bpf_prog kernel/events/core.c:8147 [inline]
_free_event+0xbdb/0x10f0 kernel/events/core.c:4116
put_event+0x24/0x30 kernel/events/core.c:4204
perf_mmap_close+0x60d/0x1010 kernel/events/core.c:5172
remove_vma+0xb4/0x1b0 mm/mmap.c:172
remove_vma_list mm/mmap.c:2490 [inline]
do_munmap+0x82a/0xdf0 mm/mmap.c:2731
mmap_region+0x59e/0x15a0 mm/mmap.c:1646
do_mmap+0x6c0/0xe00 mm/mmap.c:1483
do_mmap_pgoff include/linux/mm.h:2223 [inline]
vm_mmap_pgoff+0x1de/0x280 mm/util.c:355
SYSC_mmap_pgoff mm/mmap.c:1533 [inline]
SyS_mmap_pgoff+0x462/0x5f0 mm/mmap.c:1491
SYSC_mmap arch/x86/kernel/sys_x86_64.c:100 [inline]
SyS_mmap+0x16/0x20 arch/x86/kernel/sys_x86_64.c:91
do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x42/0xb7
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem);
lock(bpf_event_mutex);
lock(&mm->mmap_sem);
lock(bpf_event_mutex);
*** DEADLOCK ***
======================================================
The bug is introduced by Commit f371b304f12e ("bpf/tracing: allow
user space to query prog array on the same tp") where copy_to_user,
which requires mm->mmap_sem, is called inside bpf_event_mutex lock.
At the same time, during perf_event file descriptor close,
mm->mmap_sem is held first and then subsequent
perf_event_detach_bpf_prog needs bpf_event_mutex lock.
Such a senario caused a deadlock.
As suggested by Danial, moving copy_to_user out of the
bpf_event_mutex lock should fix the problem.
Fixes: f371b304f12e ("bpf/tracing: allow user space to query prog array on the same tp")
Reported-by: syzbot+dc5ca0e4c9bfafaf2bae@syzkaller.appspotmail.com
Signed-off-by: Yonghong Song <yhs@fb.com>
---
include/linux/bpf.h | 4 ++--
kernel/bpf/core.c | 25 +++++++++++++++++++------
kernel/trace/bpf_trace.c | 24 ++++++++++++++++++++----
3 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 95a7abd..486e65e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -339,8 +339,8 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
struct bpf_prog *old_prog);
int bpf_prog_array_copy_info(struct bpf_prog_array __rcu *array,
- __u32 __user *prog_ids, u32 request_cnt,
- __u32 __user *prog_cnt);
+ u32 *prog_ids, u32 request_cnt,
+ u32 *prog_cnt);
int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
struct bpf_prog *exclude_prog,
struct bpf_prog *include_prog,
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index d315b39..a95a7de 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1683,22 +1683,35 @@ int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
}
int bpf_prog_array_copy_info(struct bpf_prog_array __rcu *array,
- __u32 __user *prog_ids, u32 request_cnt,
- __u32 __user *prog_cnt)
+ u32 *prog_ids, u32 request_cnt,
+ u32 *prog_cnt)
{
- u32 cnt = 0;
+ struct bpf_prog **prog;
+ u32 i = 0, cnt = 0;
if (array)
cnt = bpf_prog_array_length(array);
- if (copy_to_user(prog_cnt, &cnt, sizeof(cnt)))
- return -EFAULT;
+ *prog_cnt = cnt;
/* return early if user requested only program count or nothing to copy */
if (!request_cnt || !cnt)
return 0;
- return bpf_prog_array_copy_to_user(array, prog_ids, request_cnt);
+ /* this function is called under trace/bpf_trace.c: bpf_event_mutex */
+ prog = rcu_dereference_check(array, 1)->progs;
+ for (; *prog; prog++) {
+ if (*prog == &dummy_bpf_prog.prog)
+ continue;
+ prog_ids[i] = (*prog)->aux->id;
+ if (++i == request_cnt) {
+ prog++;
+ break;
+ }
+ }
+ if (!!(*prog))
+ return -ENOSPC;
+ return 0;
}
static void bpf_prog_free_deferred(struct work_struct *work)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d88e96d..bd1e458 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -977,6 +977,7 @@ int perf_event_query_prog_array(struct perf_event *event, void __user *info)
{
struct perf_event_query_bpf __user *uquery = info;
struct perf_event_query_bpf query = {};
+ u32 *ids, prog_cnt, ids_len;
int ret;
if (!capable(CAP_SYS_ADMIN))
@@ -985,16 +986,31 @@ int perf_event_query_prog_array(struct perf_event *event, void __user *info)
return -EINVAL;
if (copy_from_user(&query, uquery, sizeof(query)))
return -EFAULT;
- if (query.ids_len > BPF_TRACE_MAX_PROGS)
+
+ ids_len = query.ids_len;
+ if (ids_len > BPF_TRACE_MAX_PROGS)
return -E2BIG;
+ ids = kcalloc(ids_len, sizeof(u32), GFP_USER | __GFP_NOWARN);
+ if (!ids)
+ return -ENOMEM;
mutex_lock(&bpf_event_mutex);
ret = bpf_prog_array_copy_info(event->tp_event->prog_array,
- uquery->ids,
- query.ids_len,
- &uquery->prog_cnt);
+ ids,
+ ids_len,
+ &prog_cnt);
mutex_unlock(&bpf_event_mutex);
+ if (!ret || ret == -ENOSPC) {
+ if (copy_to_user(&uquery->prog_cnt, &prog_cnt, sizeof(prog_cnt)) ||
+ copy_to_user(uquery->ids, ids, ids_len * sizeof(u32))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ }
+
+out:
+ kfree(ids);
return ret;
}
--
2.9.5
^ permalink raw reply related
* Re: [PATCH net-next] netns: filter uevents correctly
From: Christian Brauner @ 2018-04-09 15:46 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Kirill Tkhai, davem, gregkh, netdev, linux-kernel, avagin, serge
In-Reply-To: <87in953ryi.fsf@xmission.com>
On Thu, Apr 05, 2018 at 10:59:49PM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@canonical.com> writes:
>
> > On Thu, Apr 05, 2018 at 05:26:59PM +0300, Kirill Tkhai wrote:
> >> On 05.04.2018 17:07, Christian Brauner wrote:
> >> > On Thu, Apr 05, 2018 at 04:01:03PM +0300, Kirill Tkhai wrote:
> >> >> On 04.04.2018 22:48, Christian Brauner wrote:
> >> >>> commit 07e98962fa77 ("kobject: Send hotplug events in all network namespaces")
> >> >>>
> >> >>> enabled sending hotplug events into all network namespaces back in 2010.
> >> >>> Over time the set of uevents that get sent into all network namespaces has
> >> >>> shrunk. We have now reached the point where hotplug events for all devices
> >> >>> that carry a namespace tag are filtered according to that namespace.
> >> >>>
> >> >>> Specifically, they are filtered whenever the namespace tag of the kobject
> >> >>> does not match the namespace tag of the netlink socket. One example are
> >> >>> network devices. Uevents for network devices only show up in the network
> >> >>> namespaces these devices are moved to or created in.
> >> >>>
> >> >>> However, any uevent for a kobject that does not have a namespace tag
> >> >>> associated with it will not be filtered and we will *try* to broadcast it
> >> >>> into all network namespaces.
> >> >>>
> >> >>> The original patchset was written in 2010 before user namespaces were a
> >> >>> thing. With the introduction of user namespaces sending out uevents became
> >> >>> partially isolated as they were filtered by user namespaces:
> >> >>>
> >> >>> net/netlink/af_netlink.c:do_one_broadcast()
> >> >>>
> >> >>> if (!net_eq(sock_net(sk), p->net)) {
> >> >>> if (!(nlk->flags & NETLINK_F_LISTEN_ALL_NSID))
> >> >>> return;
> >> >>>
> >> >>> if (!peernet_has_id(sock_net(sk), p->net))
> >> >>> return;
> >> >>>
> >> >>> if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
> >> >>> CAP_NET_BROADCAST))
> >> >>> j return;
> >> >>> }
> >> >>>
> >> >>> The file_ns_capable() check will check whether the caller had
> >> >>> CAP_NET_BROADCAST at the time of opening the netlink socket in the user
> >> >>> namespace of interest. This check is fine in general but seems insufficient
> >> >>> to me when paired with uevents. The reason is that devices always belong to
> >> >>> the initial user namespace so uevents for kobjects that do not carry a
> >> >>> namespace tag should never be sent into another user namespace. This has
> >> >>> been the intention all along. But there's one case where this breaks,
> >> >>> namely if a new user namespace is created by root on the host and an
> >> >>> identity mapping is established between root on the host and root in the
> >> >>> new user namespace. Here's a reproducer:
> >> >>>
> >> >>> sudo unshare -U --map-root
> >> >>> udevadm monitor -k
> >> >>> # Now change to initial user namespace and e.g. do
> >> >>> modprobe kvm
> >> >>> # or
> >> >>> rmmod kvm
> >> >>>
> >> >>> will allow the non-initial user namespace to retrieve all uevents from the
> >> >>> host. This seems very anecdotal given that in the general case user
> >> >>> namespaces do not see any uevents and also can't really do anything useful
> >> >>> with them.
> >> >>>
> >> >>> Additionally, it is now possible to send uevents from userspace. As such we
> >> >>> can let a sufficiently privileged (CAP_SYS_ADMIN in the owning user
> >> >>> namespace of the network namespace of the netlink socket) userspace process
> >> >>> make a decision what uevents should be sent.
> >> >>>
> >> >>> This makes me think that we should simply ensure that uevents for kobjects
> >> >>> that do not carry a namespace tag are *always* filtered by user namespace
> >> >>> in kobj_bcast_filter(). Specifically:
> >> >>> - If the owning user namespace of the uevent socket is not init_user_ns the
> >> >>> event will always be filtered.
> >> >>> - If the network namespace the uevent socket belongs to was created in the
> >> >>> initial user namespace but was opened from a non-initial user namespace
> >> >>> the event will be filtered as well.
> >> >>> Put another way, uevents for kobjects not carrying a namespace tag are now
> >> >>> always only sent to the initial user namespace. The regression potential
> >> >>> for this is near to non-existent since user namespaces can't really do
> >> >>> anything with interesting devices.
> >> >>>
> >> >>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> >> >>> ---
> >> >>> lib/kobject_uevent.c | 10 +++++++++-
> >> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >> >>>
> >> >>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> >> >>> index 15ea216a67ce..cb98cddb6e3b 100644
> >> >>> --- a/lib/kobject_uevent.c
> >> >>> +++ b/lib/kobject_uevent.c
> >> >>> @@ -251,7 +251,15 @@ static int kobj_bcast_filter(struct sock *dsk, struct sk_buff *skb, void *data)
> >> >>> return sock_ns != ns;
> >> >>> }
> >> >>>
> >> >>> - return 0;
> >> >>> + /*
> >> >>> + * The kobject does not carry a namespace tag so filter by user
> >> >>> + * namespace below.
> >> >>> + */
> >> >>> + if (sock_net(dsk)->user_ns != &init_user_ns)
> >> >>> + return 1;
> >> >>> +
> >> >>> + /* Check if socket was opened from non-initial user namespace. */
> >> >>> + return sk_user_ns(dsk) != &init_user_ns;
> >> >>> }
> >> >>> #endif
> >> >>
> >> >> So, this prohibits to listen events of all devices except network-related
> >> >> in containers? If it's so, I don't think it's a good solution. Uevents is not
> >> >
> >> > No, this is not correct: As it is right now *without my patch* no
> >> > non-initial user namespace is receiving *any uevents* but those
> >> > specifically namespaced such as those for network devices. This patch
> >> > doesn't change that at all. The commit message outlines this in detail
> >> > how this comes about.
> >> > There is only one case where this currently breaks and this is as I
> >> > outlined explicitly in my commit message when you create a new user
> >> > namespace and map container(0) -> host(0). This patch fixes this.
> >>
> >> Could you please point the place, where non-initial user namespaces are filtered?
> >> I only see the kobj_bcast_filter() logic, and it used to return 0, which means "accepted".
> >> Now it will return 1 sometimes.
> >
> > Oh sure, it's in the commit message though. The callchain is
> > lib/kobject_uevent.c:kobject_uevent_net_broadcast() ->
> > nnet/netlink/af_netlink.c:netlink_broadcast_filtered() ->
> > net/netlink/af_netlink.c:do_one_broadcast():
> >
> > This codepiece will check whether the openened socket holds
> > CAP_NET_BROADCAST in the user namespace of the target network namespace
> > which it won't because we don't have device namespaces and all devices
> > belong to the initial set of namespaces.
> >
> > if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
> > CAP_NET_BROADCAST))
> > j return;
> >
>
> The above that only applies if someone has set NETLINK_F_LISTEN_ALL_NSID
> on their socket and has had someone with the appropriate privileges
> assign a peerrnetid.
>
> All of which is to say that file_ns_capable is not nearly as applicable
> as it might be, and if you can pass the other two checks I think it is
> pointless (because the peernet attributes are not generated for
> kobj_uevents) but valid to receive events from outside your network
> namespace.
>
>
> I might be missing something but I don't see anything excluding network
> namespaces owned by !init_user_ns excluded from the kobject_uevent
> logic.
>
> The uevent_sock_list has one entry per network namespace. And
> kobject_uevent_net_broacast appears to walk each one.
>
> I had a memory of filtering uevent messages and I had a memory
> that the netlink_has_listeners had a per network namespace effect.
> Neither seems true from my inspection of the code tonight.
>
> If we are not filtering ordinary uevents at least at the user namespace
> level that does seem to be at least a nuisance.
>
>
> Christian can you dig a little deeper into this. I have a feeling that
> there are some real efficiency improvements that we could make to
> kobject_uevent_net_broadcast if nothing else.
>
> Perhaps you could see where uevents are broadcast by poking
> the sysfs uevent of an existing device, and triggering a retransmit.
@Eric, so I did some intensive testing over the weekend and forget everything I
said before. Uevents are not filtered by the kernel at the moment. This is
currently - apart from network devices - a pure userspace thing. Specifically,
anyone on the host can listen to all uevents from everywhere. It's neither
filtered by user nor by network namespace. The fact that I used
udevadm --debug monitor
to test my prior hypothesis was what led me to believe that uevents are already
correctly filtered.
Instead, what is actually happening is that every udev implementation out there
is discarding uevents that were send by uids != 0 in the CMSG_DATA.
Specifically,
- legacy standalone udevd:
https://git.kernel.org/pub/scm/linux/hotplug/udev.git/snapshot/udev-062.tar.gz
- eudevd
https://github.com/gentoo/eudev/blob/6f630d32bf494a457171b3f99e329592497bf271/src/libudev/libudev-monitor.c#L645
- systemd-udevd
https://github.com/systemd/systemd/blob/e89ab7f219a399ab719c78cf43c07c0da60bd151/src/libudev/libudev-monitor.c#L656
- ueventd (Android)
https://android.googlesource.com/platform/system/core.git/+/android-8.1.0_r22/libcutils/uevent.c#81
For all of those I could trace this behavior back to the first released
version. (To be precise, for legacy udevd that eventually became systemd-udevd
I could trace it back to the first version that is still available on
git.kernel.org which is 062. Since eudevd is a fork of systemd-udevd it is
trivially true that it has the same behavior from the beginning.)
Android filters uevents in the same way but removed that check on January 8
2018 for what I think is invalid reasoning. The good news is that this is only
in their master branch. It hasn't even made it into an rc release for Android 8
yet. I filed a bug against Android and offered them a fix if they agree.
In any case, userspace udev is not making use of uevents at all right now since
any uid != 0 events are **explicitly** discarded.
The fact that you receive uevents for
sudo unshare -U --map-root -n
udevadm --debug monitor
is simply explained by the fact that container(0) <=> host(0) at which point
the uid in CMSG_DATA will be 0 in the new user namespace and udev will not
discard it.
The use case for receiving uevents in containers/user namespaces is definitely
there but that's what the uevent injection patch series was for that we merged.
This is a much safer and saner solution.
The fact that all udev implementations filter uevents by uid != 0 very much
seems like a security mechanism in userspace that we probably should provide by
isolating uevents based on user and/or network namespaces.
Christian
^ permalink raw reply
* Re: Enable and configure storm prevention in a network device
From: Andrew Lunn @ 2018-04-09 15:18 UTC (permalink / raw)
To: Murali Karicheri; +Cc: Florian Fainelli, David Miller, netdev
In-Reply-To: <51604140-bafe-2fd6-65b1-0a3b732c83bd@ti.com>
> > The Marvell switches have leaky buckets, which can be used for
> > limiting broadcast and multicast packets, as well as traffic shaping
> > in general. Storm prevention is just a form of traffic shaping, so if
> > we have generic traffic shaping, it can be used for storm prevention.
> >
> TI's CPSW hardware as well has similar capability to limit broadcast and
> multicast packets at the ingress. Isn't it a traffic policing at the Ingress
> rather than traffic shaping as the hardware drops the frames at the ingress
> if the rate exceeds a limit?
Hi Murali
It depends on the generation of Marvell switches. Older ones have just
egress traffic shaping. Newer ones also have ingress rate limiting.
Andrew
^ permalink raw reply
* Re: Enable and configure storm prevention in a network device
From: Murali Karicheri @ 2018-04-09 15:13 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: David Miller, netdev
In-Reply-To: <20180406143021.GK17495@lunn.ch>
Andrew,
On 04/06/2018 10:30 AM, Andrew Lunn wrote:
> On Thu, Apr 05, 2018 at 03:35:06PM -0700, Florian Fainelli wrote:
>> On 04/05/2018 01:20 PM, David Miller wrote:
>>> From: Murali Karicheri <m-karicheri2@ti.com>
>>> Date: Thu, 5 Apr 2018 16:14:49 -0400
>>>
>>>> Is there a standard way to implement and configure storm prevention
>>>> in a Linux network device?
>>>
>>> What kind of "storm", an interrupt storm?
>>>
>>
>> I would assume Murali is referring to L2 broadcast storms which is
>> common in switches. There is not an API for that AFAICT and I am not
>> sure what a proper API would look like.
>
> tc?
>
> The Marvell switches have leaky buckets, which can be used for
> limiting broadcast and multicast packets, as well as traffic shaping
> in general. Storm prevention is just a form of traffic shaping, so if
> we have generic traffic shaping, it can be used for storm prevention.
>
TI's CPSW hardware as well has similar capability to limit broadcast and
multicast packets at the ingress. Isn't it a traffic policing at the Ingress
rather than traffic shaping as the hardware drops the frames at the ingress
if the rate exceeds a limit?
I haven't done any work on TC, but with my limited knowledge of TC, it
seems we might be able to use TC to offload the TC policing to the hardware
through ndo_setup_tc()? Could someone shed some light on how to do this
with tc? Some tc filer command and then some hook in kernel to offload this
to hardware?
Murali
> Andrew
>
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* Re: [PATCH net] inetpeer: fix uninit-value in inet_getpeer
From: Eric Dumazet @ 2018-04-09 15:08 UTC (permalink / raw)
To: David Miller, edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <20180409.105839.445341256424332032.davem@davemloft.net>
On 04/09/2018 07:58 AM, David Miller wrote:
> From: Eric Dumazet <edumazet@google.com>
> Date: Mon, 9 Apr 2018 06:43:27 -0700
>
>> syzbot/KMSAN reported that p->dtime was read while it was
>> not yet initialized in :
>>
>> delta = (__u32)jiffies - p->dtime;
>> if (delta < ttl || !refcount_dec_if_one(&p->refcnt))
>> gc_stack[i] = NULL;
>>
>> This is a false positive, because the inetpeer wont be erased
>> from rb-tree if the refcount_dec_if_one(&p->refcnt) does not
>> succeed. And this wont happen before first inet_putpeer() call
>> for this inetpeer has been done, and ->dtime field is written
>> exactly before the refcount_dec_and_test(&p->refcnt).
>>
>> The KMSAN report was :
> ...
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Reported-by: syzbot <syzkaller@googlegroups.com>
>
> Applied, but it looks like we are just adding assignments simply
> to placate these reports when the tools and facilities cannot
> see through the logic properly.
>
To be fair, this is because the check on ->dtime should be done a second time after
the refcount_dec_if_one(&p->refcnt)
It is a tiny race, and we do not really care given nature of inetpeer cache, best effort,
and DDOS candidate anyway.
If we purge one entry too soon, this is not a big deal.
I believe tool is fine.
^ permalink raw reply
* Re: [PATCH] slip: Check if rstate is initialized before uncompressing
From: David Miller @ 2018-04-09 15:04 UTC (permalink / raw)
To: tejaswit; +Cc: netdev
In-Reply-To: <20180409085345.GA5972@tejaswit-linux.qualcomm.com>
From: Tejaswi Tanikella <tejaswit@codeaurora.org>
Date: Mon, 9 Apr 2018 14:23:49 +0530
> @@ -673,6 +677,7 @@ struct slcompress *
> if (cs->cs_tcp.doff > 5)
> memcpy(cs->cs_tcpopt, icp + ihl*4 + sizeof(struct tcphdr), (cs->cs_tcp.doff - 5) * 4);
> cs->cs_hsize = ihl*2 + cs->cs_tcp.doff*2;
> + cs->initialized = 1;
> /* Put headers back on packet
...
> struct cstate {
> byte_t cs_this; /* connection id number (xmit) */
> + byte_t initialized; /* non-zero if initialized */
Please use 'bool' and true/false for 'initialized'.
^ 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