* Re: [PATCH net-next v10 2/4] net: Introduce generic failover module
From: Michael S. Tsirkin @ 2018-05-11 17:15 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, jiri, kubakici, netdev,
virtualization, loseweigh, aaron.f.brown, davem
In-Reply-To: <e8454b29-d66b-9e20-a887-cb312a63847e@intel.com>
On Mon, May 07, 2018 at 05:24:27PM -0700, Samudrala, Sridhar wrote:
>
>
> On 5/7/2018 4:53 PM, Stephen Hemminger wrote:
> > On Mon, 7 May 2018 15:10:44 -0700
> > Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
> >
> > > +static struct net_device *net_failover_get_bymac(u8 *mac,
> > > + struct net_failover_ops **ops)
> > > +{
> > > + struct net_device *failover_dev;
> > > + struct net_failover *failover;
> > > +
> > > + spin_lock(&net_failover_lock);
> > > + list_for_each_entry(failover, &net_failover_list, list) {
> > > + failover_dev = rtnl_dereference(failover->failover_dev);
> > > + if (ether_addr_equal(failover_dev->perm_addr, mac)) {
> > > + *ops = rtnl_dereference(failover->ops);
> > > + spin_unlock(&net_failover_lock);
> > > + return failover_dev;
> > > + }
> > > + }
> > > + spin_unlock(&net_failover_lock);
> > > + return NULL;
> > > +}
> > This is broken if non-ethernet devices such as Infiniband are present.
>
> There is check to make sure that a slave and failover devices are of the same type in
> net_failover_slave_register()
>
> failover_dev = net_failover_get_bymac(slave_dev->perm_addr, &nfo_ops);
> if (!failover_dev)
> goto done;
>
> if (failover_dev->type != slave_dev->type)
> goto done;
>
> Do you think this is not good enough? I had an explicit check for ARPHRD_ETHER in
> earlier patchsets, but removed it based on Jiri's comment.
Right but how is ether_addr_equal supposed to work if types are
identical but not ethernet?
This can also benefit from a comment referring to the check in
net_failover_slave_register.
--
MST
^ permalink raw reply
* [PATCH v2 3/3] selinux: correctly handle sa_family cases in selinux_sctp_bind_connect()
From: Alexey Kodanev @ 2018-05-11 17:15 UTC (permalink / raw)
To: selinux
Cc: Richard Haines, Paul Moore, Stephen Smalley, Eric Paris,
linux-security-module, netdev, Alexey Kodanev
In-Reply-To: <1526058913-14198-1-git-send-email-alexey.kodanev@oracle.com>
Allow to pass the socket address structure with AF_UNSPEC family for
compatibility purposes. selinux_socket_bind() will further check it
for INADDR_ANY and selinux_socket_connect_helper() should return
EINVAL.
For a bad address family return EINVAL instead of AFNOSUPPORT error,
i.e. what is expected from SCTP protocol in such case.
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
v2: new patch in v2
security/selinux/hooks.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7882e5a..be5817d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5277,6 +5277,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
while (walk_size < addrlen) {
addr = addr_buf;
switch (addr->sa_family) {
+ case AF_UNSPEC:
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
@@ -5284,7 +5285,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
len = sizeof(struct sockaddr_in6);
break;
default:
- return -EAFNOSUPPORT;
+ return -EINVAL;
}
err = -EINVAL;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 1/3] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
From: Alexey Kodanev @ 2018-05-11 17:15 UTC (permalink / raw)
To: selinux
Cc: Richard Haines, Paul Moore, Stephen Smalley, Eric Paris,
linux-security-module, netdev, Alexey Kodanev
Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
with the old programs that can pass sockaddr_in structure with AF_UNSPEC
and INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
This was found with LTP/asapi_01 test.
Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
case to AF_INET and make sure that the address is INADDR_ANY.
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
v2: As suggested by Paul:
* return EINVAL for SCTP socket if sa_family is AF_UNSPEC and
address is not INADDR_ANY
* add new 'sa_family' variable so that it equals either AF_INET
or AF_INET6. Besides, it it will be used in the next patch that
fixes audit record.
security/selinux/hooks.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a..1ed7004 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4576,6 +4576,7 @@ static int selinux_socket_post_create(struct socket *sock, int family,
static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
{
struct sock *sk = sock->sk;
+ struct sk_security_struct *sksec = sk->sk_security;
u16 family;
int err;
@@ -4587,11 +4588,11 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
family = sk->sk_family;
if (family == PF_INET || family == PF_INET6) {
char *addrp;
- struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
+ u16 family_sa = address->sa_family;
unsigned short snum;
u32 sid, node_perm;
@@ -4601,11 +4602,20 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
* need to check address->sa_family as it is possible to have
* sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
*/
- switch (address->sa_family) {
+ switch (family_sa) {
+ case AF_UNSPEC:
case AF_INET:
if (addrlen < sizeof(struct sockaddr_in))
return -EINVAL;
addr4 = (struct sockaddr_in *)address;
+ if (family_sa == AF_UNSPEC) {
+ /* see __inet_bind(), we only want to allow
+ * AF_UNSPEC if the address is INADDR_ANY
+ */
+ if (addr4->sin_addr.s_addr != htonl(INADDR_ANY))
+ goto err_af;
+ family_sa = AF_INET;
+ }
snum = ntohs(addr4->sin_port);
addrp = (char *)&addr4->sin_addr.s_addr;
break;
@@ -4617,13 +4627,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
addrp = (char *)&addr6->sin6_addr.s6_addr;
break;
default:
- /* Note that SCTP services expect -EINVAL, whereas
- * others expect -EAFNOSUPPORT.
- */
- if (sksec->sclass == SECCLASS_SCTP_SOCKET)
- return -EINVAL;
- else
- return -EAFNOSUPPORT;
+ goto err_af;
}
if (snum) {
@@ -4681,7 +4685,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
ad.u.net->sport = htons(snum);
ad.u.net->family = family;
- if (address->sa_family == AF_INET)
+ if (family_sa == AF_INET)
ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
else
ad.u.net->v6info.saddr = addr6->sin6_addr;
@@ -4694,6 +4698,11 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
}
out:
return err;
+err_af:
+ /* Note that SCTP services expect -EINVAL, others -EAFNOSUPPORT. */
+ if (sksec->sclass == SECCLASS_SCTP_SOCKET)
+ return -EINVAL;
+ return -EAFNOSUPPORT;
}
/* This supports connect(2) and SCTP connect services such as sctp_connectx(3)
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 2/3] selinux: fix address family in bind() and connect() to match address/port
From: Alexey Kodanev @ 2018-05-11 17:15 UTC (permalink / raw)
To: selinux
Cc: Richard Haines, Paul Moore, Stephen Smalley, Eric Paris,
linux-security-module, netdev, Alexey Kodanev
In-Reply-To: <1526058913-14198-1-git-send-email-alexey.kodanev@oracle.com>
Since sctp_bindx() and sctp_connectx() can have multiple addresses,
sk_family can differ from sa_family. Therefore, selinux_socket_bind()
and selinux_socket_connect_helper(), which process sockaddr structure
(address and port), should use the address family from that structure
too, and not from the socket one.
The initialization of the data for the audit record is moved above,
in selinux_socket_bind(), so that there is no duplicate changes and
code.
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
v2: new patch in v2
security/selinux/hooks.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1ed7004..e7882e5a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4630,6 +4630,11 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
goto err_af;
}
+ ad.type = LSM_AUDIT_DATA_NET;
+ ad.u.net = &net;
+ ad.u.net->sport = htons(snum);
+ ad.u.net->family = family_sa;
+
if (snum) {
int low, high;
@@ -4641,10 +4646,6 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
snum, &sid);
if (err)
goto out;
- ad.type = LSM_AUDIT_DATA_NET;
- ad.u.net = &net;
- ad.u.net->sport = htons(snum);
- ad.u.net->family = family;
err = avc_has_perm(&selinux_state,
sksec->sid, sid,
sksec->sclass,
@@ -4676,15 +4677,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
break;
}
- err = sel_netnode_sid(addrp, family, &sid);
+ err = sel_netnode_sid(addrp, family_sa, &sid);
if (err)
goto out;
- ad.type = LSM_AUDIT_DATA_NET;
- ad.u.net = &net;
- ad.u.net->sport = htons(snum);
- ad.u.net->family = family;
-
if (family_sa == AF_INET)
ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
else
@@ -4780,7 +4776,7 @@ static int selinux_socket_connect_helper(struct socket *sock,
ad.type = LSM_AUDIT_DATA_NET;
ad.u.net = &net;
ad.u.net->dport = htons(snum);
- ad.u.net->family = sk->sk_family;
+ ad.u.net->family = address->sa_family;
err = avc_has_perm(&selinux_state,
sksec->sid, sid, sksec->sclass, perm, &ad);
if (err)
--
1.8.3.1
^ permalink raw reply related
* Re: KASAN: null-ptr-deref Read in rds_ib_get_mr
From: Santosh Shilimkar @ 2018-05-11 16:58 UTC (permalink / raw)
To: Yanjun Zhu, DaeRyong Jeong, davem
Cc: netdev, linux-rdma, rds-devel, linux-kernel, byoungyoung, kt0755
In-Reply-To: <fa3461d4-8872-48af-9b67-be0affd16bbd@oracle.com>
On 5/11/2018 12:48 AM, Yanjun Zhu wrote:
>
>
> On 2018/5/11 13:20, DaeRyong Jeong wrote:
>> We report the crash: KASAN: null-ptr-deref Read in rds_ib_get_mr
>>
>> Note that this bug is previously reported by syzkaller.
>> https://syzkaller.appspot.com/bug?id=0bb56a5a48b000b52aa2b0d8dd20b1f545214d91
>>
>> Nonetheless, this bug has not fixed yet, and we hope that this report
>> and our
>> analysis, which gets help by the RaceFuzzer's feature, will helpful to
>> fix the
>> crash.
>>
>> This crash has been found in v4.17-rc1 using RaceFuzzer (a modified
>> version of Syzkaller), which we describe more at the end of this
>> report. Our analysis shows that the race occurs when invoking two
>> syscalls concurrently, bind$rds and setsockopt$RDS_GET_MR.
>>
>>
>> Analysis:
>> We think the concurrent execution of __rds_rdma_map() and rds_bind()
>> causes the problem. __rds_rdma_map() checks whether rs->rs_bound_addr
>> is 0
>> or not. But the concurrent execution with rds_bind() can by-pass this
>> check. Therefore, __rds_rdmap_map() calls rs->rs_transport->get_mr() and
>> rds_ib_get_mr() causes the null deref at ib_rdma.c:544 in v4.17-rc1, when
>> dereferencing rs_conn.
>>
>>
>> Thread interleaving:
>> CPU0 (__rds_rdma_map) CPU1 (rds_bind)
>> // rds_add_bound() sets rs->bound_addr as
>> none 0
>> ret = rds_add_bound(rs,
>> sin->sin_addr.s_addr, &sin->sin_port);
>> if (rs->rs_bound_addr == 0 || !rs->rs_transport) {
>> ret = -ENOTCONN; /* XXX not a great errno */
>> goto out;
>> }
>> if (rs->rs_transport) { /* previously
>> bound */
>> trans = rs->rs_transport;
>> if
>> (trans->laddr_check(sock_net(sock->sk),
>> sin->sin_addr.s_addr)
>> != 0) {
>> ret = -ENOPROTOOPT;
>> // rds_remove_bound() sets
>> rs->bound_addr as 0
>> rds_remove_bound(rs);
>> ...
>> trans_private = rs->rs_transport->get_mr(sg, nents, rs,
>> &mr->r_key);
>> (in rds_ib_get_mr())
>> struct rds_ib_connection *ic = rs->rs_conn->c_transport_data;
>>
>>
>> Call sequence (v4.17-rc1):
>> CPU0
>> rds_setsockopt
>> rds_get_mr
>> __rds_rdma_map
>> rds_ib_get_mr
>>
>>
>> CPU1
>> rds_bind
>> rds_add_bound
>> ...
>> rds_remove_bound
>>
>>
>> Crash log:
>> ==================================================================
>> BUG: KASAN: null-ptr-deref in rds_ib_get_mr+0x3a/0x150
>> net/rds/ib_rdma.c:544
>> Read of size 8 at addr 0000000000000068 by task syz-executor0/32067
>>
>> CPU: 0 PID: 32067 Comm: syz-executor0 Not tainted 4.17.0-rc1 #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x166/0x21c lib/dump_stack.c:113
>> kasan_report_error mm/kasan/report.c:352 [inline]
>> kasan_report+0x140/0x360 mm/kasan/report.c:412
>> check_memory_region_inline mm/kasan/kasan.c:260 [inline]
>> __asan_load8+0x54/0x90 mm/kasan/kasan.c:699
>> rds_ib_get_mr+0x3a/0x150 net/rds/ib_rdma.c:544
>> __rds_rdma_map+0x521/0x9d0 net/rds/rdma.c:271
>> rds_get_mr+0xad/0xf0 net/rds/rdma.c:333
>> rds_setsockopt+0x57f/0x720 net/rds/af_rds.c:347
>> __sys_setsockopt+0x147/0x230 net/socket.c:1903
>> __do_sys_setsockopt net/socket.c:1914 [inline]
>> __se_sys_setsockopt net/socket.c:1911 [inline]
>> __x64_sys_setsockopt+0x67/0x80 net/socket.c:1911
>> do_syscall_64+0x15f/0x4a0 arch/x86/entry/common.c:287
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x4563f9
>> RSP: 002b:00007f6a2b3c2b28 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
>> RAX: ffffffffffffffda RBX: 000000000072bee0 RCX: 00000000004563f9
>> RDX: 0000000000000002 RSI: 0000000000000114 RDI: 0000000000000015
>> RBP: 0000000000000575 R08: 0000000000000020 R09: 0000000000000000
>> R10: 0000000020000140 R11: 0000000000000246 R12: 00007f6a2b3c36d4
>> R13: 00000000ffffffff R14: 00000000006fd398 R15: 0000000000000000
>> ==================================================================
> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index e678699..2228b50 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -539,11 +539,17 @@ void rds_ib_flush_mrs(void)
> void *rds_ib_get_mr(struct scatterlist *sg, unsigned long nents,
> struct rds_sock *rs, u32 *key_ret)
> {
> - struct rds_ib_device *rds_ibdev;
> + struct rds_ib_device *rds_ibdev = NULL;
> struct rds_ib_mr *ibmr = NULL;
> - struct rds_ib_connection *ic = rs->rs_conn->c_transport_data;
> + struct rds_ib_connection *ic = NULL;
> int ret;
>
> + if (rs->rs_bound_addr == 0) {
> + ret = -EPERM;
> + goto out;
> + }
> +
No you can't return such error for this API and the
socket related checks needs to be done at core layer.
I remember fixing this race but probably never pushed
fix upstream.
The MR code is due for update with optimized FRWR code
which now stable enough. We will address this issue as
well as part of that patchset.
Thanks for looking into it.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH bpf-next] samples/bpf: xdp_monitor, accept short options
From: Jesper Dangaard Brouer @ 2018-05-11 16:31 UTC (permalink / raw)
To: Prashant Bhole
Cc: Daniel Borkmann, Alexei Starovoitov, David S . Miller, netdev,
brouer
In-Reply-To: <20180511013751.4360-1-bhole_prashant_q7@lab.ntt.co.jp>
On Fri, 11 May 2018 10:37:51 +0900
Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp> wrote:
> updated optstring accept short options
>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
> samples/bpf/xdp_monitor_user.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/samples/bpf/xdp_monitor_user.c b/samples/bpf/xdp_monitor_user.c
> index 894bc64c2cac..668511c77aaf 100644
> --- a/samples/bpf/xdp_monitor_user.c
> +++ b/samples/bpf/xdp_monitor_user.c
> @@ -594,7 +594,7 @@ int main(int argc, char **argv)
> snprintf(bpf_obj_file, sizeof(bpf_obj_file), "%s_kern.o", argv[0]);
>
> /* Parse commands line args */
> - while ((opt = getopt_long(argc, argv, "h",
> + while ((opt = getopt_long(argc, argv, "hDSs:",
> long_options, &longindex)) != -1) {
> switch (opt) {
> case 'D':
It was actually on purpose that I didn't add the short options,
in-order to force people use those "self-documenting" long-options when
they show the usage on public mailing lists or in blog-posts.
If you want these short options, you also have to correct the "usage"
function that state these are "internal" short-options.
Notice the long options parsing done by getopt_long() allow you to only
specify part of the string. Al-through, I can see --s is ambiguous.
$ sudo ./xdp_monitor --s
./xdp_monitor: option '--s' is ambiguous; possibilities: '--stats' '--sec'
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next] udp: avoid refcount_t saturation in __udp_gso_segment()
From: David Miller @ 2018-05-11 16:30 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, willemb, alexander.h.duyck
In-Reply-To: <20180511020713.159465-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 10 May 2018 19:07:13 -0700
> For some reason, Willem thought that the issue we fixed for TCP
> in commit 7ec318feeed1 ("tcp: gso: avoid refcount_t warning from
> tcp_gso_segment()") was not relevant for UDP GSO.
>
> But syzbot found its way.
...
> Fixes: ad405857b174 ("udp: better wmem accounting on gso")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [resend PATCH] rxrpc: Neaten logging macros and add KERN_DEBUG logging level
From: Joe Perches @ 2018-05-11 16:29 UTC (permalink / raw)
To: David Howells; +Cc: David S. Miller, linux-afs, netdev, linux-kernel
In-Reply-To: <35831b4769a0415ae7b975e88badb3033dbfe82d.1522176274.git.joe@perches.com>
On Tue, 2018-03-27 at 11:52 -0700, Joe Perches wrote:
> When enabled, the current debug logging does not have a KERN_<LEVEL>.
> Add KERN_DEBUG to the logging macros.
>
> Miscellanea:
>
> o Remove #define redundancy and neaten the macros a bit
ping?
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> Resend of patch: https://lkml.org/lkml/2017/11/30/573
>
> No change in patch.
>
> David Howells is now a listed maintainer for net/rxrpc/ so he should receive
> this patch via get_maintainer
>
> net/rxrpc/ar-internal.h | 75 ++++++++++++++++++-------------------------------
> 1 file changed, 28 insertions(+), 47 deletions(-)
>
> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> index 416688381eb7..d4b53b2339b3 100644
> --- a/net/rxrpc/ar-internal.h
> +++ b/net/rxrpc/ar-internal.h
> @@ -1147,66 +1147,47 @@ static inline bool after_eq(u32 seq1, u32 seq2)
> */
> extern unsigned int rxrpc_debug;
>
> -#define dbgprintk(FMT,...) \
> - printk("[%-6.6s] "FMT"\n", current->comm ,##__VA_ARGS__)
> +#if defined(__KDEBUG) || defined(CONFIG_AF_RXRPC_DEBUG)
> +#define dbgprintk(fmt, ...) \
> + printk(KERN_DEBUG "[%-6.6s] " fmt "\n", current->comm, ##__VA_ARGS__)
> +#else
> +#define dbgprintk(fmt, ...) \
> + no_printk(KERN_DEBUG "[%-6.6s] " fmt "\n", current->comm, ##__VA_ARGS__)
> +#endif
>
> -#define kenter(FMT,...) dbgprintk("==> %s("FMT")",__func__ ,##__VA_ARGS__)
> -#define kleave(FMT,...) dbgprintk("<== %s()"FMT"",__func__ ,##__VA_ARGS__)
> -#define kdebug(FMT,...) dbgprintk(" "FMT ,##__VA_ARGS__)
> -#define kproto(FMT,...) dbgprintk("### "FMT ,##__VA_ARGS__)
> -#define knet(FMT,...) dbgprintk("@@@ "FMT ,##__VA_ARGS__)
> +#define kenter(fmt, ...) dbgprintk("==> %s(" fmt ")", __func__, ##__VA_ARGS__)
> +#define kleave(fmt, ...) dbgprintk("<== %s()" fmt "", __func__, ##__VA_ARGS__)
> +#define kdebug(fmt, ...) dbgprintk(" " fmt, ##__VA_ARGS__)
> +#define kproto(fmt, ...) dbgprintk("### " fmt, ##__VA_ARGS__)
> +#define knet(fmt, ...) dbgprintk("@@@ " fmt, ##__VA_ARGS__)
>
> +#if defined(__KDEBUG) || !defined(CONFIG_AF_RXRPC_DEBUG)
> +#define _enter(fmt, ...) kenter(fmt, ##__VA_ARGS__)
> +#define _leave(fmt, ...) kleave(fmt, ##__VA_ARGS__)
> +#define _debug(fmt, ...) kdebug(fmt, ##__VA_ARGS__)
> +#define _proto(fmt, ...) kproto(fmt, ##__VA_ARGS__)
> +#define _net(fmt, ...) knet(fmt, ##__VA_ARGS__)
>
> -#if defined(__KDEBUG)
> -#define _enter(FMT,...) kenter(FMT,##__VA_ARGS__)
> -#define _leave(FMT,...) kleave(FMT,##__VA_ARGS__)
> -#define _debug(FMT,...) kdebug(FMT,##__VA_ARGS__)
> -#define _proto(FMT,...) kproto(FMT,##__VA_ARGS__)
> -#define _net(FMT,...) knet(FMT,##__VA_ARGS__)
> +#else
>
> -#elif defined(CONFIG_AF_RXRPC_DEBUG)
> #define RXRPC_DEBUG_KENTER 0x01
> #define RXRPC_DEBUG_KLEAVE 0x02
> #define RXRPC_DEBUG_KDEBUG 0x04
> #define RXRPC_DEBUG_KPROTO 0x08
> #define RXRPC_DEBUG_KNET 0x10
>
> -#define _enter(FMT,...) \
> -do { \
> - if (unlikely(rxrpc_debug & RXRPC_DEBUG_KENTER)) \
> - kenter(FMT,##__VA_ARGS__); \
> -} while (0)
> -
> -#define _leave(FMT,...) \
> -do { \
> - if (unlikely(rxrpc_debug & RXRPC_DEBUG_KLEAVE)) \
> - kleave(FMT,##__VA_ARGS__); \
> -} while (0)
> -
> -#define _debug(FMT,...) \
> -do { \
> - if (unlikely(rxrpc_debug & RXRPC_DEBUG_KDEBUG)) \
> - kdebug(FMT,##__VA_ARGS__); \
> -} while (0)
> -
> -#define _proto(FMT,...) \
> -do { \
> - if (unlikely(rxrpc_debug & RXRPC_DEBUG_KPROTO)) \
> - kproto(FMT,##__VA_ARGS__); \
> +#define RXRPC_DEBUG(TYPE, type, fmt, ...) \
> +do { \
> + if (unlikely(rxrpc_debug & RXRPC_DEBUG_##TYPE)) \
> + type(fmt, ##__VA_ARGS__); \
> } while (0)
>
> -#define _net(FMT,...) \
> -do { \
> - if (unlikely(rxrpc_debug & RXRPC_DEBUG_KNET)) \
> - knet(FMT,##__VA_ARGS__); \
> -} while (0)
> +#define _enter(fmt, ...) RXRPC_DEBUG(KENTER, kenter, fmt, ##__VA_ARGS__)
> +#define _leave(fmt, ...) RXRPC_DEBUG(KLEAVE, kleave, fmt, ##__VA_ARGS__)
> +#define _debug(fmt, ...) RXRPC_DEBUG(KDEBUG, kdebug, fmt, ##__VA_ARGS__)
> +#define _proto(fmt, ...) RXRPC_DEBUG(KPROTO, kproto, fmt, ##__VA_ARGS__)
> +#define _net(fmt, ...) RXRPC_DEBUG(KNET, knet, fmt, ##__VA_ARGS__)
>
> -#else
> -#define _enter(FMT,...) no_printk("==> %s("FMT")",__func__ ,##__VA_ARGS__)
> -#define _leave(FMT,...) no_printk("<== %s()"FMT"",__func__ ,##__VA_ARGS__)
> -#define _debug(FMT,...) no_printk(" "FMT ,##__VA_ARGS__)
> -#define _proto(FMT,...) no_printk("### "FMT ,##__VA_ARGS__)
> -#define _net(FMT,...) no_printk("@@@ "FMT ,##__VA_ARGS__)
> #endif
>
> /*
^ permalink raw reply
* Re: [pull request][net 0/3] Mellanox, mlx5 fixes 2018-05-10
From: David Miller @ 2018-05-11 16:27 UTC (permalink / raw)
To: saeedm; +Cc: netdev
In-Reply-To: <20180510231915.23754-1-saeedm@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Thu, 10 May 2018 16:19:12 -0700
> the following series includes some fixes for mlx5 core driver.
> Please pull and let me know if there's any problem.
Pulled.
> For -stable v4.5
> ("net/mlx5: E-Switch, Include VF RDMA stats in vport statistics")
>
> For -stable v4.10
> ("net/mlx5e: Err if asked to offload TC match on frag being first")
Queued up for -stable.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 net-next] tcp: switch pacing timer to softirq based hrtimer
From: David Miller @ 2018-05-11 16:24 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <20180510215943.94513-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 10 May 2018 14:59:43 -0700
> linux-4.16 got support for softirq based hrtimers.
> TCP can switch its pacing hrtimer to this variant, since this
> avoids going through a tasklet and some atomic operations.
>
> pacing timer logic looks like other (jiffies based) tcp timers.
>
> v2: use hrtimer_try_to_cancel() in tcp_clear_xmit_timers()
> to correctly release reference on socket if needed.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Looks great, applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH net-next v2 0/9] net: dsa: Plug in PHYLINK support
From: David Miller @ 2018-05-11 16:23 UTC (permalink / raw)
To: f.fainelli
Cc: netdev, privat, andrew, vivien.didelot, rmk+kernel, sean.wang,
Woojung.Huh, john, cphealy
In-Reply-To: <20180510201737.13887-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 10 May 2018 13:17:28 -0700
> This patch series adds PHYLINK support to DSA which is necessary to
> support more complex PHY and pluggable modules setups.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] coredump: rename umh_pipe_setup() to coredump_pipe_setup()
From: Luis R. Rodriguez @ 2018-05-11 16:17 UTC (permalink / raw)
To: Al Viro, David S. Miller
Cc: Luis R. Rodriguez, Alexei Starovoitov, ast, linux-fsdevel,
linux-kernel, netdev
In-Reply-To: <20180511024851.GB30522@ZenIV.linux.org.uk>
On Fri, May 11, 2018 at 03:48:51AM +0100, Al Viro wrote:
> On Thu, May 10, 2018 at 11:32:47PM +0000, Luis R. Rodriguez wrote:
>
> > I think net-next makes sense if Al Viro is OK with that. This way it could go
> > in regardless of the state of your series, but it also lines up with your work.
>
> Fine by me...
OK thanks.
Dave, I'll bounce a copy of the original patch to you, if anything else is needed
please let me know.
Luis
^ permalink raw reply
* Re: [PATCH net] ipv4: fix memory leaks in udp_sendmsg, ping_v4_sendmsg
From: David Miller @ 2018-05-11 16:01 UTC (permalink / raw)
To: rdna; +Cc: netdev, ast, eric.dumazet, kernel-team
In-Reply-To: <20180510175934.2259802-1-rdna@fb.com>
From: Andrey Ignatov <rdna@fb.com>
Date: Thu, 10 May 2018 10:59:34 -0700
> Fix more memory leaks in ip_cmsg_send() callers. Part of them were fixed
> earlier in 919483096bfe.
>
> * udp_sendmsg one was there since the beginning when linux sources were
> first added to git;
> * ping_v4_sendmsg one was copy/pasted in c319b4d76b9e.
>
> Whenever return happens in udp_sendmsg() or ping_v4_sendmsg() IP options
> have to be freed if they were allocated previously.
>
> Add label so that future callers (if any) can use it instead of kfree()
> before return that is easy to forget.
>
> Fixes: c319b4d76b9e (net: ipv4: add IPPROTO_ICMP socket kind)
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Applied and queued up for -stable, thank you.
^ permalink raw reply
* Re: [PATCH] mlxsw: core: Fix an error handling path in 'mlxsw_core_bus_device_register()'
From: David Miller @ 2018-05-11 15:57 UTC (permalink / raw)
To: idosch
Cc: christophe.jaillet, jiri, idosch, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <20180510115821.GA10270@splinter.mtl.com>
From: Ido Schimmel <idosch@idosch.org>
Date: Thu, 10 May 2018 14:58:21 +0300
> On Thu, May 10, 2018 at 01:26:16PM +0200, Christophe JAILLET wrote:
>> Resources are not freed in the reverse order of the allocation.
>> Labels are also mixed-up.
>>
>> Fix it and reorder code and labels in the error handling path of
>> 'mlxsw_core_bus_device_register()'
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> For net:
>
> Fixes: ef3116e5403e ("mlxsw: spectrum: Register KVD resources with devlink")
> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net 0/2] bonding: bug fixes and regressions
From: David Miller @ 2018-05-11 15:51 UTC (permalink / raw)
To: dbanerje; +Cc: netdev, vyasevic, j.vosburgh, vfalico, andy
In-Reply-To: <20180509233211.28207-1-dbanerje@akamai.com>
From: Debabrata Banerjee <dbanerje@akamai.com>
Date: Wed, 9 May 2018 19:32:09 -0400
> Fixes to bonding driver for balance-alb mode, suitable for stable.
Series applied and queued up for -stable, thanks.
^ permalink raw reply
* [patch net] net: sched: fix error path in tcf_proto_create() when modules are not configured
From: Jiri Pirko @ 2018-05-11 15:45 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, mlxsw
From: Jiri Pirko <jiri@mellanox.com>
In case modules are not configured, error out when tp->ops is null
and prevent later null pointer dereference.
Fixes: 33a48927c193 ("sched: push TC filter protocol creation into a separate function")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index b66754f52a9f..963e4bf0aab8 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -152,8 +152,8 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
NL_SET_ERR_MSG(extack, "TC classifier not found");
err = -ENOENT;
}
- goto errout;
#endif
+ goto errout;
}
tp->classify = tp->ops->classify;
tp->protocol = protocol;
--
2.14.3
^ permalink raw reply related
* Re: [PATCH net-next v10 2/4] net: Introduce generic failover module
From: Samudrala, Sridhar @ 2018-05-11 15:43 UTC (permalink / raw)
To: Randy Dunlap, mst, stephen, davem, netdev, virtualization,
virtio-dev, jesse.brandeburg, alexander.h.duyck, kubakici,
jasowang, loseweigh, jiri, aaron.f.brown
In-Reply-To: <460f3d8f-b2ec-2118-e296-03f4f9655c5a@infradead.org>
On 5/7/2018 3:39 PM, Randy Dunlap wrote:
> Hi,
>
> On 05/07/2018 03:10 PM, Sridhar Samudrala wrote:
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>> MAINTAINERS | 7 +
>> include/linux/netdevice.h | 16 +
>> include/net/net_failover.h | 52 +++
>> net/Kconfig | 10 +
>> net/core/Makefile | 1 +
>> net/core/net_failover.c | 1044 ++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 1130 insertions(+)
>> create mode 100644 include/net/net_failover.h
>> create mode 100644 net/core/net_failover.c
>
>> diff --git a/net/Kconfig b/net/Kconfig
>> index b62089fb1332..0540856676de 100644
>> --- a/net/Kconfig
>> +++ b/net/Kconfig
>> @@ -429,6 +429,16 @@ config MAY_USE_DEVLINK
>> config PAGE_POOL
>> bool
>>
>> +config NET_FAILOVER
>> + tristate "Failover interface"
>> + default m
> Need some justification for default m (as opposed to n).
default n should be fine. It will get selected automatically when virtio_net or
netvsc are enabled. will fix in the next revision.
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v10 2/4] net: Introduce generic failover module
From: Samudrala, Sridhar @ 2018-05-11 15:40 UTC (permalink / raw)
To: Stephen Hemminger
Cc: mst, davem, netdev, virtualization, virtio-dev, jesse.brandeburg,
alexander.h.duyck, kubakici, jasowang, loseweigh, jiri,
aaron.f.brown
In-Reply-To: <20180507164632.4f6c2eef@xeon-e3>
On 5/7/2018 4:46 PM, Stephen Hemminger wrote:
> On Mon, 7 May 2018 15:10:44 -0700
> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>
>> This provides a generic interface for paravirtual drivers to listen
>> for netdev register/unregister/link change events from pci ethernet
>> devices with the same MAC and takeover their datapath. The notifier and
>> event handling code is based on the existing netvsc implementation.
>>
>> It exposes 2 sets of interfaces to the paravirtual drivers.
>> 1. For paravirtual drivers like virtio_net that use 3 netdev model, the
>> the failover module provides interfaces to create/destroy additional
>> master netdev and all the slave events are managed internally.
>> net_failover_create()
>> net_failover_destroy()
>> A failover netdev is created that acts a master device and controls 2
>> slave devices. The original virtio_net netdev is registered as 'standby'
>> netdev and a passthru/vf device with the same MAC gets registered as
>> 'primary' netdev. Both 'standby' and 'failover' netdevs are associated
>> with the same 'pci' device. The user accesses the network interface via
>> 'failover' netdev. The 'failover' netdev chooses 'primary' netdev as
>> default for transmits when it is available with link up and running.
>> 2. For existing netvsc driver that uses 2 netdev model, no master netdev
>> is created. The paravirtual driver registers each instance of netvsc
>> as a 'failover' netdev along with a set of ops to manage the slave
>> events. There is no 'standby' netdev in this model. A passthru/vf device
>> with the same MAC gets registered as 'primary' netdev.
>> net_failover_register()
>> net_failover_unregister()
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> You are conflating the net_failover device (3 device model) with
> the generic network failover infrastructure into one file. There should be two
> seperate files net/core/failover.c and drivers/net/failover.c which splits
> the work into two parts (and acts a check for the api).
OK. I started splitting net_failover.c into 2 files.
net/core/failover.c (CONFIG_FAILOVER)
- implements the generic failover infrastructure that exports failover_register(),
failover_unregister() and failover_slave_unregister() as the API that will be
used by netvsc and the net_failover drivers(3 netdev model)
drivers/net/net_failover.c (CONFIG_NET_FAILOVER)
- implements the net_failover netdev as the upper dev for the 3-netdev model and
exports net_failover_create() and net_failover_destroy() as the API that is
used by virtio_net.
HYPERV_NET and NET_FAILOVER selects FAILOVER
VIRTIO_NET selects NET_FAILOVER
Does this look good? Any better suggestion for the prefix to be used for generic
network failover api rather than 'failover'?
^ permalink raw reply
* Re: [RFC] net: Add new LoRaWAN subsystem
From: Marcel Holtmann @ 2018-05-11 15:39 UTC (permalink / raw)
To: Jian-Hong Pan
Cc: David S. Miller, Alexander Aring, Stefan Schmidt, linux-wpan - ML,
netdev, linux-kernel
In-Reply-To: <CAC=mGzijVyqEG=DXH4v9WkD0kXR2WOJC4KBPzoT7g5wqVrPGXA@mail.gmail.com>
Hi Jian-Hong,
> A Low-Power Wide-Area Network (LPWAN) is a type of wireless
> telecommunication wide area network designed to allow long range
> communications at a low bit rate among things (connected objects), such
> as sensors operated on a battery. It can be used widely in IoT area.
> LoRaWAN, which is one kind of implementation of LPWAN, is a medium
> access control (MAC) layer protocol for managing communication between
> LPWAN gateways and end-node devices, maintained by the LoRa Alliance.
> LoRaWAN™ Specification could be downloaded at:
> https://lora-alliance.org/lorawan-for-developers
>
> However, LoRaWAN is not implemented in Linux kernel right now, so I am
> trying to develop it. Here is my repository:
> https://github.com/starnight/LoRa/tree/lorawan-ndo/LoRaWAN
>
> Because it is a kind of network, the ideal usage in an user space
> program should be like "socket(PF_LORAWAN, SOCK_DGRAM, 0)" and with
> other socket APIs. Therefore, the definitions like AF_LORAWAN,
> PF_LORAWAN ..., must be listed in the header files of glibc.
> For the driver in kernel space, the definitions also must be listed in
> the corresponding Linux socket header files.
> Especially, both are for the testing programs.
>
> Back to the mentioned "LoRaWAN is not implemented in Linux kernel now".
> Could or should we add the definitions into corresponding kernel header
> files now, if LoRaWAN will be accepted as a subsystem in Linux?
when you submit your LoRaWAN subsystem to netdev for review, include a patch that adds these new address family definitions. Just pick the next one available. There will be no pre-allocation of numbers until your work has been accepted upstream. Meaning, that the number might change if other address families get merged before yours. So you have to keep updating. glibc will eventually follow the number assigned by the kernel.
Regards
Marcel
^ permalink raw reply
* Re: [RFC bpf-next 04/10] bpf: cfg: detect loop use domination information
From: Jiong Wang @ 2018-05-11 15:11 UTC (permalink / raw)
To: John Fastabend, alexei.starovoitov, daniel; +Cc: netdev, oss-drivers
In-Reply-To: <06c6f498-2e85-e762-09b6-766612c0bc46@gmail.com>
On 10/05/2018 19:17, John Fastabend wrote:
> On 05/07/2018 03:22 AM, Jiong Wang wrote:
>> If one bb is dominating its predecessor, then there is loop.
>>
>> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
>> ---
>> kernel/bpf/cfg.c | 22 ++++++++++++++++++++++
>> kernel/bpf/cfg.h | 1 +
>> kernel/bpf/verifier.c | 8 ++++++++
>> 3 files changed, 31 insertions(+)
>>
>> diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
>> index b50937a..90692e4 100644
>> --- a/kernel/bpf/cfg.c
>> +++ b/kernel/bpf/cfg.c
>> @@ -568,6 +568,28 @@ int subprog_build_dom_info(struct bpf_subprog_info *subprog)
>> return ret;
>> }
>>
>> +bool subprog_has_loop(struct bpf_subprog_info *subprog)
>> +{
>> + int lane_len = BITS_TO_LONGS(subprog->bb_num - 2);
>> + struct list_head *bb_list = &subprog->bbs;
>> + struct bb_node *bb, *entry_bb;
>> + struct edge_node *e;
>> +
>> + entry_bb = entry_bb(bb_list);
>> + bb = bb_next(entry_bb);
>> + list_for_each_entry_from(bb, &exit_bb(bb_list)->l, l)
>> + list_for_each_entry(e, &bb->e_prevs, l) {
>> + struct bb_node *latch = e->src;
>> +
>> + if (latch != entry_bb &&
>> + test_bit(bb->idx,
>> + subprog->dtree + latch->idx * lane_len))
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
> Because we are using this to guard against loops we need to detect
> all loops not just reducible loops. And because (assuming my understanding
> is correct) Tarjan's algorithm will only detect all loops when the
> graph is reducible we need additional tests.
Hi John,
Yes, the current DOM based loop detection can't detect irreducible loop.
And I feel both the first and second approaches you listed below are good,
might worth implementing both and measure which one is with less overhead.
I could give a try on approach 2. The alg given by Eric Stoltz might be a
good choice (https://compilers.iecc.com/comparch/article/94-01-053). We
have dom info now, so could do another DFS, and see if the head of one
back-edge doesn't dom the tail that there is multiple entries to the loop.
I guess the first approach is with less overhead as there is no existing
DFS pass to reuse after dom build that we need a new pass for approach 2.
Regards,
Jiong
>
> There are a couple options to fix this with varying levels of complexity.
> Because I'm using this to build loop info structures to find induction
> variables and show termination. After the loop structures are built we
> could search for any back-edges not in valid loops. This would be similar
> to the existing back-edge detection code but with an extra check to
> allow edges that have been validated. I would need to check that this
> doesn't have any escapes before actually proposing it though.
>
> The other method would be to properly test for reducibility using one of
> the algorithms for this. I think the most intuitive is to remove back-edges
> and test the graph is acyclic. This would be run before the dom tree is
> built. This is IMO what we should do, it seems the most "correct" way to
> do this.
>
> The most complex would be to handle irreducible programs using some of the
> more complex methods. I really don't think this is necessary but in theory
> at least we could use something like Havlak-Tarjan algorithm and allow
> some programs with irreducible loops. This is likely overkill especially
> in a first iteration.
>
> Here is a sample that fails without this series, using original back-edge
> detection, but is allowed with this patch,
>
> SEC("classifier_tc_mark")
> int _tc_mark(struct __sk_buff *ctx)
> {
> void *data = (void *)(unsigned long)ctx->data;
> void *data_end = (void *)(unsigned long)ctx->data_end;
> void *data_meta = (void *)(unsigned long)ctx->data_meta;
> struct meta_info *meta = data_meta;
> volatile int mark = ctx->mark;
>
> mark += 1;
>
> if (meta + 1 > data) {
> B:
> mark += 2;
>
> if (mark < ctx->mark * 3)
> goto C;
> } else if (meta < data) {
> C:
> mark += 1;
> if (mark < 1000)
> goto B;
> }
>
> return TC_ACT_OK;
> }
>
> A more concise example could be made but I just hacked on one of the
> sample programs. This generates the CFG as follows (I have a patch
> on top of your stack to print the CFG and DOM tables)
>
> CFG: 65535[-1,-1] -> 0[0,9] 0[0,9] -> 3[20,20] 0[0,9] -> 1[10,18] 1[10,18] -> 4[21,28] 1[10,18] -> 2[19,19] 2[19,19] -> 5[29,30] 3[20,20] -> 5[29,30] 3[20,20] -> 4[21,28] 4[21,28] -> 1[10,18] 4[21,28] -> 5[29,30] 5[29,30] -> 65534[31,65534]
> DOM:
> 1 0 0 0 0 0
> 1 1 0 0 0 0
> 1 1 1 0 0 0
> 1 0 0 1 0 0
> 1 0 0 0 1 0
> 1 0 0 0 0 1
>
>
> Here we have the loop 1[10,18]->4[21,28] and the back-edge 4[21,28]->1[10,18].
> The notation is #idx[head_insn,tail_insn]. The above can then be imported
> into dot notation and graphed if needed.
>
> Jiong, please verify this analysis is correct.
>
> Thanks,
> John
>
^ permalink raw reply
* Re: Re: Re: [PATCH net] net: Correct wrong skb_flow_limit check when enable RPS
From: Willem de Bruijn @ 2018-05-11 14:56 UTC (permalink / raw)
To: Gao Feng
Cc: davem@davemloft.net, daniel@iogearbox.net,
jakub.kicinski@netronome.com, David Ahern, netdev@vger.kernel.org
In-Reply-To: <ac2b5b7.97a1.1634fa8f60d.Coremail.gfree.wind@vip.163.com>
On Fri, May 11, 2018 at 10:44 AM, Gao Feng <gfree.wind@vip.163.com> wrote:
> At 2018-05-11 21:23:55, "Willem de Bruijn" <willemdebruijn.kernel@gmail.com> wrote:
>>On Fri, May 11, 2018 at 2:20 AM, Gao Feng <gfree.wind@vip.163.com> wrote:
>>> At 2018-05-11 11:54:55, "Willem de Bruijn" <willemdebruijn.kernel@gmail.com> wrote:
>>>>On Thu, May 10, 2018 at 4:28 AM, <gfree.wind@vip.163.com> wrote:
>>>>> From: Gao Feng <gfree.wind@vip.163.com>
>>>>>
>>>>> The skb flow limit is implemented for each CPU independently. In the
>>>>> current codes, the function skb_flow_limit gets the softnet_data by
>>>>> this_cpu_ptr. But the target cpu of enqueue_to_backlog would be not
>>>>> the current cpu when enable RPS. As the result, the skb_flow_limit checks
>>>>> the stats of current CPU, while the skb is going to append the queue of
>>>>> another CPU. It isn't the expected behavior.
>>>>>
>>>>> Now pass the softnet_data as a param to softnet_data to make consistent.
>>>>
>>>>The local cpu softnet_data is used on purpose. The operations in
>>>>skb_flow_limit() on sd fields could race if not executed on the local cpu.
>>>
>>> I think the race doesn't exist because of the rps_lock.
>>> The enqueue_to_backlog has hold the rps_lock before skb_flow_limit.
>>
>>Indeed, I overlooked that. There still is the matter of cache contention.
>
> The cache contention is really important in this case?
> I don't think so, because the enqueue_to_backlog have touched and modified the softnet_stat
> of target cpu.
>
>>
>>>>Flow limit tries to detect large ("elephant") DoS flows with a fixed four-tuple.
>>>>These would always hit the same RPS cpu, so that cpu being backlogged
>>>
>>> They may hit the different target CPU when enable RFS. Because the app could be scheduled
>>> to another CPU, then RFS tries to deliver the skb to latest core which has hot cache.
>>
>>This even more suggest using the initial (or IRQ) cpu to track state, instead
>>of the destination (RPS/RFS) cpu.
>
> I couldn't understand why it is better to track state on initial cpu, not the target cpu.
> The latter one could get more accurate result.
For a single DoS flow with normal cpu pinned IRQs, the results will be equally
good when tracked on the initial IRQ cpu..
>
>>
>>>>may be an indication that such a flow is active. But the flow will also always
>>>>arrive on the same initial cpu courtesy of RSS. So storing the lookup table
>>>
>>> The RSS couldn't make sure the irq is handled by same cpu. It would be balanced between
>>> the cpus.
>>
>>IRQs are usually pinned to cores. Unless using something like irqbalance,
>>but that operates at too coarse a timescale to do anything useful at Mpps
>>packet rates.
>
> There are some motherboard which couldn't make sure the irq is pinned.
> The flow_limit wouldn't work as well as expected.
.. this seems to be the crux of the argument. I am not aware of any network
interrupts that do not adhere to the cpu pinning configuration in
/proc/irq/$IRQ/smp_affinity(_list)
What kind of hardware ignores this setting and sprays interrupts? I agree
that in that case flow_limit as is may be ineffective (if migration happens
at rates comparable to packet rates). But this should not happen?
>
>>
>>>>on the initial CPU is also fine. There may be false positives on other CPUs
>>>>with the same RPS destination, but that is unlikely with a highly concurrent
>>>>traffic server mix ("mice").
>>>
>>> If my comment is right, the flow couldn't always arrive one the same initial cpu, although
>>> it may be sent to one same target cpu.
>>>
>>>>
>>>>Note that the sysctl net.core.flow_limit_cpu_bitmap enables the feature
>>>>for the cpus on which traffic initially lands, not the RPS destination cpus.
>>>>See also Documentation/networking/scaling.txt
>>>>
>>>>That said, I had to reread the code, as it does seem sensible that the
>>>>same softnet_data is intended to be used both when testing qlen and
>>>>flow_limit.
>>>
>>> In most cases, user configures the same RPS map with flow_limit like 0xff.
>>> Because user couldn't predict which core the evil flow would arrive on.
>>>
>>> Take an example, there are 2 cores, cpu0 and cpu1.
>>> One flow is the an evil flow, but the irq is sent to cpu0. After RPS/RFS, the target cpu is cpu1.
>>> Now cpu0 invokes enqueue_to_backlog, then the skb_flow_limit checkes the queue length
>>> of cpu0. Certainly it could pass the check of skb_flow_limit because there is no any evil flow on cpu0.
>>
>>No, enqueue_to_backlog passes qlen to skb_flow_limit, so that does
>>check the queue length of the RPS cpu.
>
> Sorry, I overlooked the qlen is the length of the rps cpu.
> Then it's ok unless the stats may be not accurate when irq isn't pinned.
>
> But I still doubt that is it really important to track state on initial cpu, not target cpu?
> Because the enqueue_to_backlog have touched the softnet_data of target cpu.
I think the merit of both IRQ and RPS cpu can be argued for attaching the
flow_limit state.
Either way, the current behavior is not a bug, so I don't think that this is a
candidate for net.
The cost of moving from IRQ to RPS cpu will be the cacheline contention
on a system with multiple IRQ cpus that all try to update the sd->flow_data
of the same RPS cpus. Which is particularly likely with RFS. I suspect that
this cost is non-trivial and not worth the benefit of handling hardware with
unpinned IRQs.
^ permalink raw reply
* [PATCH v2 net 1/1] net sched actions: fix invalid pointer dereferencing if skbedit flags missing
From: Roman Mashak @ 2018-05-11 14:55 UTC (permalink / raw)
To: davem
Cc: netdev, kernel, jhs, xiyou.wangcong, jiri, alexander.duyck,
Roman Mashak
When application fails to pass flags in netlink TLV for a new skbedit action,
the kernel results in the following oops:
[ 8.307732] BUG: unable to handle kernel paging request at 0000000000021130
[ 8.309167] PGD 80000000193d1067 P4D 80000000193d1067 PUD 180e0067 PMD 0
[ 8.310595] Oops: 0000 [#1] SMP PTI
[ 8.311334] Modules linked in: kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel aes_x86_64 crypto_simd cryptd glue_helper serio_raw
[ 8.314190] CPU: 1 PID: 397 Comm: tc Not tainted 4.17.0-rc3+ #357
[ 8.315252] RIP: 0010:__tcf_idr_release+0x33/0x140
[ 8.316203] RSP: 0018:ffffa0718038f840 EFLAGS: 00010246
[ 8.317123] RAX: 0000000000000001 RBX: 0000000000021100 RCX: 0000000000000000
[ 8.319831] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000021100
[ 8.321181] RBP: 0000000000000000 R08: 000000000004adf8 R09: 0000000000000122
[ 8.322645] R10: 0000000000000000 R11: ffffffff9e5b01ed R12: 0000000000000000
[ 8.324157] R13: ffffffff9e0d3cc0 R14: 0000000000000000 R15: 0000000000000000
[ 8.325590] FS: 00007f591292e700(0000) GS:ffff8fcf5bc40000(0000) knlGS:0000000000000000
[ 8.327001] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 8.327987] CR2: 0000000000021130 CR3: 00000000180e6004 CR4: 00000000001606a0
[ 8.329289] Call Trace:
[ 8.329735] tcf_skbedit_init+0xa7/0xb0
[ 8.330423] tcf_action_init_1+0x362/0x410
[ 8.331139] ? try_to_wake_up+0x44/0x430
[ 8.331817] tcf_action_init+0x103/0x190
[ 8.332511] tc_ctl_action+0x11a/0x220
[ 8.333174] rtnetlink_rcv_msg+0x23d/0x2e0
[ 8.333902] ? _cond_resched+0x16/0x40
[ 8.334569] ? __kmalloc_node_track_caller+0x5b/0x2c0
[ 8.335440] ? rtnl_calcit.isra.31+0xf0/0xf0
[ 8.336178] netlink_rcv_skb+0xdb/0x110
[ 8.336855] netlink_unicast+0x167/0x220
[ 8.337550] netlink_sendmsg+0x2a7/0x390
[ 8.338258] sock_sendmsg+0x30/0x40
[ 8.338865] ___sys_sendmsg+0x2c5/0x2e0
[ 8.339531] ? pagecache_get_page+0x27/0x210
[ 8.340271] ? filemap_fault+0xa2/0x630
[ 8.340943] ? page_add_file_rmap+0x108/0x200
[ 8.341732] ? alloc_set_pte+0x2aa/0x530
[ 8.342573] ? finish_fault+0x4e/0x70
[ 8.343332] ? __handle_mm_fault+0xbc1/0x10d0
[ 8.344337] ? __sys_sendmsg+0x53/0x80
[ 8.345040] __sys_sendmsg+0x53/0x80
[ 8.345678] do_syscall_64+0x4f/0x100
[ 8.346339] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 8.347206] RIP: 0033:0x7f591191da67
[ 8.347831] RSP: 002b:00007fff745abd48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 8.349179] RAX: ffffffffffffffda RBX: 00007fff745abe70 RCX: 00007f591191da67
[ 8.350431] RDX: 0000000000000000 RSI: 00007fff745abdc0 RDI: 0000000000000003
[ 8.351659] RBP: 000000005af35251 R08: 0000000000000001 R09: 0000000000000000
[ 8.352922] R10: 00000000000005f1 R11: 0000000000000246 R12: 0000000000000000
[ 8.354183] R13: 00007fff745afed0 R14: 0000000000000001 R15: 00000000006767c0
[ 8.355400] Code: 41 89 d4 53 89 f5 48 89 fb e8 aa 20 fd ff 85 c0 0f 84 ed 00
00 00 48 85 db 0f 84 cf 00 00 00 40 84 ed 0f 85 cd 00 00 00 45 84 e4 <8b> 53 30
74 0d 85 d2 b8 ff ff ff ff 0f 8f b3 00 00 00 8b 43 2c
[ 8.358699] RIP: __tcf_idr_release+0x33/0x140 RSP: ffffa0718038f840
[ 8.359770] CR2: 0000000000021130
[ 8.360438] ---[ end trace 60c66be45dfc14f0 ]---
The caller calls action's ->init() and passes pointer to "struct tc_action *a",
which later may be initialized to point at the existing action, otherwise
"struct tc_action *a" is still invalid, and therefore dereferencing it is an
error as happens in tcf_idr_release, where refcnt is decremented.
So in case of missing flags tcf_idr_release must be called only for
existing actions.
v2:
- prepare patch for net tree
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
net/sched/act_skbedit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index ddf69fc01bdf..6138d1d71900 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -121,7 +121,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
return 0;
if (!flags) {
- tcf_idr_release(*a, bind);
+ if (exists)
+ tcf_idr_release(*a, bind);
return -EINVAL;
}
--
2.7.4
^ permalink raw reply related
* Re:Re: Re: [PATCH net] net: Correct wrong skb_flow_limit check when enable RPS
From: Gao Feng @ 2018-05-11 14:44 UTC (permalink / raw)
To: Willem de Bruijn
Cc: davem@davemloft.net, daniel@iogearbox.net,
jakub.kicinski@netronome.com, David Ahern, netdev@vger.kernel.org
In-Reply-To: <CAF=yD-JuAivOyrimVSxB_TtaiyiT498Z6fpe+XpF_oNqCMdUPA@mail.gmail.com>
At 2018-05-11 21:23:55, "Willem de Bruijn" <willemdebruijn.kernel@gmail.com> wrote:
>On Fri, May 11, 2018 at 2:20 AM, Gao Feng <gfree.wind@vip.163.com> wrote:
>> At 2018-05-11 11:54:55, "Willem de Bruijn" <willemdebruijn.kernel@gmail.com> wrote:
>>>On Thu, May 10, 2018 at 4:28 AM, <gfree.wind@vip.163.com> wrote:
>>>> From: Gao Feng <gfree.wind@vip.163.com>
>>>>
>>>> The skb flow limit is implemented for each CPU independently. In the
>>>> current codes, the function skb_flow_limit gets the softnet_data by
>>>> this_cpu_ptr. But the target cpu of enqueue_to_backlog would be not
>>>> the current cpu when enable RPS. As the result, the skb_flow_limit checks
>>>> the stats of current CPU, while the skb is going to append the queue of
>>>> another CPU. It isn't the expected behavior.
>>>>
>>>> Now pass the softnet_data as a param to softnet_data to make consistent.
>>>
>>>The local cpu softnet_data is used on purpose. The operations in
>>>skb_flow_limit() on sd fields could race if not executed on the local cpu.
>>
>> I think the race doesn't exist because of the rps_lock.
>> The enqueue_to_backlog has hold the rps_lock before skb_flow_limit.
>
>Indeed, I overlooked that. There still is the matter of cache contention.
The cache contention is really important in this case?
I don't think so, because the enqueue_to_backlog have touched and modified the softnet_stat
of target cpu.
>
>>>Flow limit tries to detect large ("elephant") DoS flows with a fixed four-tuple.
>>>These would always hit the same RPS cpu, so that cpu being backlogged
>>
>> They may hit the different target CPU when enable RFS. Because the app could be scheduled
>> to another CPU, then RFS tries to deliver the skb to latest core which has hot cache.
>
>This even more suggest using the initial (or IRQ) cpu to track state, instead
>of the destination (RPS/RFS) cpu.
I couldn't understand why it is better to track state on initial cpu, not the target cpu.
The latter one could get more accurate result.
>
>>>may be an indication that such a flow is active. But the flow will also always
>>>arrive on the same initial cpu courtesy of RSS. So storing the lookup table
>>
>> The RSS couldn't make sure the irq is handled by same cpu. It would be balanced between
>> the cpus.
>
>IRQs are usually pinned to cores. Unless using something like irqbalance,
>but that operates at too coarse a timescale to do anything useful at Mpps
>packet rates.
There are some motherboard which couldn't make sure the irq is pinned.
The flow_limit wouldn't work as well as expected.
>
>>>on the initial CPU is also fine. There may be false positives on other CPUs
>>>with the same RPS destination, but that is unlikely with a highly concurrent
>>>traffic server mix ("mice").
>>
>> If my comment is right, the flow couldn't always arrive one the same initial cpu, although
>> it may be sent to one same target cpu.
>>
>>>
>>>Note that the sysctl net.core.flow_limit_cpu_bitmap enables the feature
>>>for the cpus on which traffic initially lands, not the RPS destination cpus.
>>>See also Documentation/networking/scaling.txt
>>>
>>>That said, I had to reread the code, as it does seem sensible that the
>>>same softnet_data is intended to be used both when testing qlen and
>>>flow_limit.
>>
>> In most cases, user configures the same RPS map with flow_limit like 0xff.
>> Because user couldn't predict which core the evil flow would arrive on.
>>
>> Take an example, there are 2 cores, cpu0 and cpu1.
>> One flow is the an evil flow, but the irq is sent to cpu0. After RPS/RFS, the target cpu is cpu1.
>> Now cpu0 invokes enqueue_to_backlog, then the skb_flow_limit checkes the queue length
>> of cpu0. Certainly it could pass the check of skb_flow_limit because there is no any evil flow on cpu0.
>
>No, enqueue_to_backlog passes qlen to skb_flow_limit, so that does
>check the queue length of the RPS cpu.
Sorry, I overlooked the qlen is the length of the rps cpu.
Then it's ok unless the stats may be not accurate when irq isn't pinned.
But I still doubt that is it really important to track state on initial cpu, not target cpu?
Because the enqueue_to_backlog have touched the softnet_data of target cpu.
Best Regards
Feng
>
>> Then the cpu0 inserts the skb into the queue of cpu1.
>> As a result, the skb_flow_limit doesn't work as expected.
>>
>> BTW, I have already sent the v2 patch which only adds the "Fixes: tag".
>
>The change also makes the code inconsistent with
>Documentation/networking/scaling.txt
>
>"In such environments, enable the feature on all CPUs that handle
>network rx interrupts (as set in /proc/irq/N/smp_affinity)."
^ permalink raw reply
* Re: WARNING in compat_copy_entries (2)
From: Dmitry Vyukov @ 2018-05-11 14:13 UTC (permalink / raw)
To: syzbot
Cc: bridge, coreteam, David Miller, Florian Westphal,
Jozsef Kadlecsik, LKML, netdev, netfilter-devel,
Pablo Neira Ayuso, stephen hemminger, syzkaller-bugs
In-Reply-To: <001a113dea0ef7d2620566c15bac@google.com>
On Tue, Mar 6, 2018 at 5:59 PM, syzbot
<syzbot+659574e7bcc7f7eb4df7@syzkaller.appspotmail.com> wrote:
> Hello,
>
> syzbot hit the following crash on upstream commit
> ce380619fab99036f5e745c7a865b21c59f005f6 (Tue Mar 6 04:31:14 2018 +0000)
> Merge tag 'please-pull-ia64_misc' of
> git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux
>
> So far this crash happened 11 times on upstream.
> syzkaller reproducer is attached.
> Raw console output is attached.
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached.
> user-space arch: i386
This was bisected to, well:
commit 9dea5dc921b5f4045a18c63eb92e84dc274d17eb
Author: Andy Lutomirski <luto@kernel.org>
Date: Tue Jul 14 15:24:24 2015 -0700
x86/entry/syscalls: Wire up 32-bit direct socket calls
https://gist.githubusercontent.com/dvyukov/acbdace00cbb2fd31fb599a009d471e6/raw/bfe6bd8f2423df7b3493c7df2e466b37a8968688/gistfile1.txt
Which is kinda makes sense, but shows another limitation of automatic bisection.
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+659574e7bcc7f7eb4df7@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> audit: type=1400 audit(1520347101.934:8): avc: denied { map } for
> pid=4183 comm="syz-execprog" path="/root/syzkaller-shm886349459" dev="sda1"
> ino=16482 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
> tcontext=unconfined_u:object_r:file_t:s0 tclass=file permissive=1
> IPVS: ftp: loaded support on port[0] = 21
> WARNING: CPU: 1 PID: 4192 at net/bridge/netfilter/ebtables.c:2063
> ebt_size_mwt net/bridge/netfilter/ebtables.c:2063 [inline]
> WARNING: CPU: 1 PID: 4192 at net/bridge/netfilter/ebtables.c:2063
> size_entry_mwt net/bridge/netfilter/ebtables.c:2140 [inline]
> WARNING: CPU: 1 PID: 4192 at net/bridge/netfilter/ebtables.c:2063
> compat_copy_entries+0xd92/0x1150 net/bridge/netfilter/ebtables.c:2179
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 1 PID: 4192 Comm: syz-executor0 Not tainted 4.16.0-rc4+ #253
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:17 [inline]
> dump_stack+0x194/0x24d lib/dump_stack.c:53
> panic+0x1e4/0x41c kernel/panic.c:183
> __warn+0x1dc/0x200 kernel/panic.c:547
> report_bug+0x211/0x2d0 lib/bug.c:184
> fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178
> fixup_bug arch/x86/kernel/traps.c:247 [inline]
> do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296
> do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315
> invalid_op+0x1b/0x40 arch/x86/entry/entry_64.S:986
> RIP: 0010:ebt_size_mwt net/bridge/netfilter/ebtables.c:2063 [inline]
> RIP: 0010:size_entry_mwt net/bridge/netfilter/ebtables.c:2140 [inline]
> RIP: 0010:compat_copy_entries+0xd92/0x1150
> net/bridge/netfilter/ebtables.c:2179
> RSP: 0018:ffff8801bb5077e8 EFLAGS: 00010293
> RAX: ffff8801bc82a580 RBX: 0000000000000000 RCX: ffffffff851ad5c2
> RDX: 0000000000000000 RSI: ffffc90001853180 RDI: 0000000000000000
> RBP: ffff8801bb507968 R08: 0000000000000004 R09: 0000000000000000
> R10: ffffffff88613380 R11: 0000000000000001 R12: 0000000000000007
> R13: dffffc0000000000 R14: ffff8801bb5079c8 R15: 000000000000000c
> compat_do_replace+0x398/0x7d0 net/bridge/netfilter/ebtables.c:2268
> compat_do_ebt_set_ctl+0x22a/0x2d0 net/bridge/netfilter/ebtables.c:2350
> compat_nf_sockopt net/netfilter/nf_sockopt.c:144 [inline]
> compat_nf_setsockopt+0x88/0x130 net/netfilter/nf_sockopt.c:156
> compat_ip_setsockopt+0x8b/0xd0 net/ipv4/ip_sockglue.c:1285
> inet_csk_compat_setsockopt+0x95/0x120 net/ipv4/inet_connection_sock.c:1041
> compat_dccp_setsockopt+0x40/0x70 net/dccp/proto.c:589
> compat_sock_common_setsockopt+0xb2/0x140 net/core/sock.c:2986
> C_SYSC_setsockopt net/compat.c:403 [inline]
> compat_SyS_setsockopt+0x17c/0x410 net/compat.c:386
> do_syscall_32_irqs_on arch/x86/entry/common.c:330 [inline]
> do_fast_syscall_32+0x3ec/0xf9f arch/x86/entry/common.c:392
> entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
> RIP: 0023:0xf7f35c99
> RSP: 002b:00000000ff9c02cc EFLAGS: 00000282 ORIG_RAX: 000000000000016e
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000000000
> RDX: 0000000000000080 RSI: 0000000020000240 RDI: 0000000000000240
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
>
>
> ---
> This bug is generated by a dumb bot. It may contain errors.
> See https://goo.gl/tpsmEJ for details.
> Direct all questions to syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report.
> If you forgot to add the Reported-by tag, once the fix for this bug is
> merged
> into any tree, please reply to this email with:
> #syz fix: exact-commit-title
> If you want to test a patch for this bug, please reply with:
> #syz test: git://repo/address.git branch
> and provide the patch inline or as an attachment.
> To mark this as a duplicate of another syzbot report, please reply with:
> #syz dup: exact-subject-of-another-report
> If it's a one-off invalid bug report, please reply with:
> #syz invalid
> Note: if the crash happens again, it will cause creation of a new bug
> report.
> Note: all commands must start from beginning of the line in the email body.
>
> --
> You received this message because you are subscribed to the Google Groups
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/syzkaller-bugs/001a113dea0ef7d2620566c15bac%40google.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [rds-devel] KASAN: null-ptr-deref Read in rds_ib_get_mr
From: Yanjun Zhu @ 2018-05-11 14:07 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: DaeRyong Jeong, santosh.shilimkar, davem, rds-devel, kt0755,
linux-rdma, netdev, linux-kernel, byoungyoung
In-Reply-To: <20180511104630.GD14952@oracle.com>
On 2018/5/11 18:46, Sowmini Varadhan wrote:
> On (05/11/18 15:48), Yanjun Zhu wrote:
>> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
>> index e678699..2228b50 100644
>> --- a/net/rds/ib_rdma.c
>> +++ b/net/rds/ib_rdma.c
>> @@ -539,11 +539,17 @@ void rds_ib_flush_mrs(void)
>> void *rds_ib_get_mr(struct scatterlist *sg, unsigned long nents,
>> struct rds_sock *rs, u32 *key_ret)
>> {
>> - struct rds_ib_device *rds_ibdev;
>> + struct rds_ib_device *rds_ibdev = NULL;
>> struct rds_ib_mr *ibmr = NULL;
>> - struct rds_ib_connection *ic = rs->rs_conn->c_transport_data;
>> + struct rds_ib_connection *ic = NULL;
>> int ret;
>>
>> + if (rs->rs_bound_addr == 0) {
>> + ret = -EPERM;
>> + goto out;
>> + }
>> +
>> + ic = rs->rs_conn->c_transport_data;
>> rds_ibdev = rds_ib_get_device(rs->rs_bound_addr);
>> if (!rds_ibdev) {
>> ret = -ENODEV;
>>
>> I made this raw patch. If you can reproduce this bug, please make tests
>> with it.
> I dont think this solves the problem, I think it
> just changes the timing under which it can still happen.
>
> what if the rds_remove_bound() in rds_bind() happens after the check
> for if (rs->rs_bound_addr == 0) added above by the patch
>
> I believe you need some type of synchronization (either
> through mutex, or some atomic flag in the rs or similar) to make
> sure rds_bind() and rds_ib_get_mr() are mutually exclusive.
Sure. I agree with you. Maybe mutex is a good choice.
Zhu Yanjun
>
> --Sowmini
>
>
>
^ 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