* Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv
From: Alexei Starovoitov @ 2017-06-08 4:38 UTC (permalink / raw)
To: Haishuang Yan; +Cc: =David S. Miller, netdev, Eric Dumazet
In-Reply-To: <1496896364-27153-2-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
> dst_release to free it in error code path.
>
> CC: Alexei Starovoitov <ast@fb.com>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
I don't get it. Why did you send another version of the patch?
What was wrong with previous approach that myself and Eric acked?
^ permalink raw reply
* [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv
From: Haishuang Yan @ 2017-06-08 4:32 UTC (permalink / raw)
To: =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, linux-kernel, Haishuang Yan, Alexei Starovoitov
In-Reply-To: <1496896364-27153-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
dst_release to free it in error code path.
CC: Alexei Starovoitov <ast@fb.com>
Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
Changes in v2:
- Add the the missing Fixes information
Changes in v3:
- Free tun_dst from error code path
---
net/ipv6/ip6_tunnel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 9b37f97..ef99d59 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -859,6 +859,8 @@ static int __ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
return 0;
drop:
+ if (tun_dst)
+ dst_release((struct dst_entry *)tun_dst);
kfree_skb(skb);
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv
From: Haishuang Yan @ 2017-06-08 4:32 UTC (permalink / raw)
To: =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, linux-kernel, Haishuang Yan, Pravin B Shelar
When ip_tunnel_rcv fails, the tun_dst won't be freed, so call
dst_release to free it in error code path.
CC: Pravin B Shelar <pshelar@nicira.com>
Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
Changes in v2:
- Add the the missing Fixes information
Changes in v3:
- Free tun_dst from error code path
---
net/ipv4/ip_tunnel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index b878ecb..0f1d876 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -446,6 +446,8 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
return 0;
drop:
+ if (tun_dst)
+ dst_release((struct dst_entry *)tun_dst);
kfree_skb(skb);
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv
From: Pravin Shelar @ 2017-06-08 4:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: Haishuang Yan, =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy,
Linux Kernel Network Developers, linux-kernel, Pravin B Shelar,
Haishuang Yan
In-Reply-To: <1496891711.736.55.camel@edumazet-glaptop3.roam.corp.google.com>
On Wed, Jun 7, 2017 at 8:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2017-06-07 at 19:13 -0700, Pravin Shelar wrote:
>> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
>> <yanhaishuang@cmss.chinamobile.com> wrote:
>> > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
>> > skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>> >
>> > CC: Pravin B Shelar <pshelar@nicira.com>
>> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
>> > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
>> > ---
>> > net/ipv4/ip_tunnel.c | 6 +++---
>> > 1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> > index b878ecb..27fc20f 100644
>> > --- a/net/ipv4/ip_tunnel.c
>> > +++ b/net/ipv4/ip_tunnel.c
>> > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>> > const struct iphdr *iph = ip_hdr(skb);
>> > int err;
>> >
>> > + if (tun_dst)
>> > + skb_dst_set(skb, (struct dst_entry *)tun_dst);
>> > +
>> If dst is set so early, skb_scrub_packet() would remove the tunnel dst
>> reference.
>> It is better to call skb_dst_drop() from error code path.
>
> Do we really want to keep a dst from another namespace if
> skb_scrub_packet() is called with xnet=true ?
>
tun_dst is allocated in same namespace. It is required for LWT to work.
^ permalink raw reply
* [PATCH net-next 3/3] LiquidIO: lowmem: init allocated memory to 0
From: Felix Manlunas @ 2017-06-08 3:53 UTC (permalink / raw)
To: davem
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
ricardo.farrington
In-Reply-To: <20170608035045.GA1608@felix-thinkpad.cavium.com>
From: Rick Farrington <ricardo.farrington@cavium.com>
For defensive programming, zero the allocated block 'oct->droq[0]' in
octeon_setup_output_queues() and 'oct->instr_queue[0]' in
octeon_setup_instr_queues().
Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
Signed-off-by: Satanand Burla <satananda.burla@cavium.com>
Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
---
drivers/net/ethernet/cavium/liquidio/octeon_device.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
index 3b7cc93..087820b 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -880,11 +880,11 @@ int octeon_setup_instr_queues(struct octeon_device *oct)
oct->num_iqs = 0;
- oct->instr_queue[0] = vmalloc_node(sizeof(*oct->instr_queue[0]),
+ oct->instr_queue[0] = vzalloc_node(sizeof(*oct->instr_queue[0]),
numa_node);
if (!oct->instr_queue[0])
oct->instr_queue[0] =
- vmalloc(sizeof(struct octeon_instr_queue));
+ vzalloc(sizeof(struct octeon_instr_queue));
if (!oct->instr_queue[0])
return 1;
memset(oct->instr_queue[0], 0, sizeof(struct octeon_instr_queue));
@@ -927,9 +927,9 @@ int octeon_setup_output_queues(struct octeon_device *oct)
desc_size = CFG_GET_DEF_RX_BUF_SIZE(CHIP_CONF(oct, cn23xx_vf));
}
oct->num_oqs = 0;
- oct->droq[0] = vmalloc_node(sizeof(*oct->droq[0]), numa_node);
+ oct->droq[0] = vzalloc_node(sizeof(*oct->droq[0]), numa_node);
if (!oct->droq[0])
- oct->droq[0] = vmalloc(sizeof(*oct->droq[0]));
+ oct->droq[0] = vzalloc(sizeof(*oct->droq[0]));
if (!oct->droq[0])
return 1;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 2/3] LiquidIO: lowmem: do not dereference NULL ptr
From: Felix Manlunas @ 2017-06-08 3:52 UTC (permalink / raw)
To: davem
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
ricardo.farrington
In-Reply-To: <20170608035045.GA1608@felix-thinkpad.cavium.com>
From: Rick Farrington <ricardo.farrington@cavium.com>
Don't dereference a NULL ptr in octeon_droq_destroy_ring_buffers().
Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
Signed-off-by: Satanand Burla <satananda.burla@cavium.com>
Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
---
drivers/net/ethernet/cavium/liquidio/octeon_droq.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_droq.c b/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
index 07c764d..d868252 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
@@ -145,6 +145,8 @@ static void octeon_droq_reset_indices(struct octeon_droq *droq)
for (i = 0; i < droq->max_count; i++) {
pg_info = &droq->recv_buf_list[i].pg_info;
+ if (!pg_info)
+ continue;
if (pg_info->dma)
lio_unmap_ring(oct->pci_dev,
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 1/3] LiquidIO: lowmem: init allocated memory to 0
From: Felix Manlunas @ 2017-06-08 3:52 UTC (permalink / raw)
To: davem
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
ricardo.farrington
In-Reply-To: <20170608035045.GA1608@felix-thinkpad.cavium.com>
From: Rick Farrington <ricardo.farrington@cavium.com>
Fix GPF in octeon_init_droq(); zero the allocated block 'recv_buf_list'.
This prevents a GPF trying to access an invalid 'recv_buf_list[i]' entry
in octeon_droq_destroy_ring_buffers() if init didn't alloc all entries.
Signed-off-by: Rick Farrington <ricardo.farrington@cavium.com>
Signed-off-by: Satanand Burla <satananda.burla@cavium.com>
Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
---
drivers/net/ethernet/cavium/liquidio/octeon_droq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_droq.c b/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
index 286be55..07c764d 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_droq.c
@@ -289,12 +289,12 @@ int octeon_init_droq(struct octeon_device *oct,
}
droq->recv_buf_list = (struct octeon_recv_buffer *)
- vmalloc_node(droq->max_count *
+ vzalloc_node(droq->max_count *
OCT_DROQ_RECVBUF_SIZE,
numa_node);
if (!droq->recv_buf_list)
droq->recv_buf_list = (struct octeon_recv_buffer *)
- vmalloc(droq->max_count *
+ vzalloc(droq->max_count *
OCT_DROQ_RECVBUF_SIZE);
if (!droq->recv_buf_list) {
dev_err(&oct->pci_dev->dev, "Output queue recv buf list alloc failed\n");
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 0/3] LiquidIO: avoid VM low memory crashes
From: Felix Manlunas @ 2017-06-08 3:50 UTC (permalink / raw)
To: davem
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
ricardo.farrington
From: Rick Farrington <ricardo.farrington@cavium.com>
This patchset addresses issues brought about by low memory conditions
in a VM. These conditions were only seen when VM's were intentionally
created with small amounts of memory (for stress testing).
1. Fix GPF in octeon_init_droq(); zero the allocated block 'recv_buf_list'.
This prevents a GPF trying to access an invalid 'recv_buf_list[i]' entry
in octeon_droq_destroy_ring_buffers() if init didn't alloc all entries.
2. Don't dereference a NULL ptr in octeon_droq_destroy_ring_buffers().
3. For defensive programming, zero the allocated block 'oct->droq[0]' in
octeon_setup_output_queues() and 'oct->instr_queue[0]' in
octeon_setup_instr_queues().
drivers/net/ethernet/cavium/liquidio/octeon_device.c | 8 ++++----
drivers/net/ethernet/cavium/liquidio/octeon_droq.c | 6 ++++--
2 files changed, 8 insertions(+), 6 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv
From: Eric Dumazet @ 2017-06-08 3:15 UTC (permalink / raw)
To: Pravin Shelar
Cc: Haishuang Yan, =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy,
Linux Kernel Network Developers, linux-kernel, Pravin B Shelar,
Haishuang Yan
In-Reply-To: <CAOrHB_CZ-3TRs2O3KATW_ji_QQxqO=28UeDU_-Gx7ZPxz_RWCg@mail.gmail.com>
On Wed, 2017-06-07 at 19:13 -0700, Pravin Shelar wrote:
> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
> <yanhaishuang@cmss.chinamobile.com> wrote:
> > When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
> > skb_dst_set to begin and tun_dst would be freed by kfree_skb.
> >
> > CC: Pravin B Shelar <pshelar@nicira.com>
> > Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
> > ---
> > net/ipv4/ip_tunnel.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> > index b878ecb..27fc20f 100644
> > --- a/net/ipv4/ip_tunnel.c
> > +++ b/net/ipv4/ip_tunnel.c
> > @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
> > const struct iphdr *iph = ip_hdr(skb);
> > int err;
> >
> > + if (tun_dst)
> > + skb_dst_set(skb, (struct dst_entry *)tun_dst);
> > +
> If dst is set so early, skb_scrub_packet() would remove the tunnel dst
> reference.
> It is better to call skb_dst_drop() from error code path.
Do we really want to keep a dst from another namespace if
skb_scrub_packet() is called with xnet=true ?
^ permalink raw reply
* (unknown),
From: agar2000 @ 2017-06-08 3:14 UTC (permalink / raw)
To: netdev
[-- Attachment #1: 225162210782.zip --]
[-- Type: application/zip, Size: 3145 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv
From: 严海双 @ 2017-06-08 3:07 UTC (permalink / raw)
To: Pravin Shelar
Cc: =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy,
Linux Kernel Network Developers, LKML, Pravin B Shelar
In-Reply-To: <CAOrHB_CZ-3TRs2O3KATW_ji_QQxqO=28UeDU_-Gx7ZPxz_RWCg@mail.gmail.com>
> On 8 Jun 2017, at 10:13 AM, Pravin Shelar <pshelar@ovn.org> wrote:
>
> On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
> <yanhaishuang@cmss.chinamobile.com> wrote:
>> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
>> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>>
>> CC: Pravin B Shelar <pshelar@nicira.com>
>> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
>> ---
>> net/ipv4/ip_tunnel.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> index b878ecb..27fc20f 100644
>> --- a/net/ipv4/ip_tunnel.c
>> +++ b/net/ipv4/ip_tunnel.c
>> @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>> const struct iphdr *iph = ip_hdr(skb);
>> int err;
>>
>> + if (tun_dst)
>> + skb_dst_set(skb, (struct dst_entry *)tun_dst);
>> +
> If dst is set so early, skb_scrub_packet() would remove the tunnel dst
> reference.
> It is better to call skb_dst_drop() from error code path.
Yes, I will change it in v3 commit, thanks!
>
>> #ifdef CONFIG_NET_IPGRE_BROADCAST
>> if (ipv4_is_multicast(iph->daddr)) {
>> tunnel->dev->stats.multicast++;
>> @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>> skb->dev = tunnel->dev;
>> }
>>
>> - if (tun_dst)
>> - skb_dst_set(skb, (struct dst_entry *)tun_dst);
>> -
>> gro_cells_receive(&tunnel->gro_cells, skb);
>> return 0;
>>
>> --
>> 1.8.3.1
>>
>>
>>
>
^ permalink raw reply
* Re: linux-next: build failure after merge of the wireless-drivers-next tree
From: Stephen Rothwell @ 2017-06-08 2:57 UTC (permalink / raw)
To: Igor Mitsyanko
Cc: Kalle Valo, Wireless, David Miller, Networking,
Linux-Next Mailing List, Linux Kernel Mailing List, Dmitrii Lebed,
Sergei Maksimenko, Sergey Matyukevich, Bindu Therthala,
Huizhao Wang, Kamlesh Rath, Avinash Patil
In-Reply-To: <3594bd0d-e2b4-a380-ba06-8dedbbcbe11f@quantenna.com>
Hi Igor,
On Wed, 7 Jun 2017 19:43:18 -0700 Igor Mitsyanko <igor.mitsyanko.os@quantenna.com> wrote:
>
> thanks. As I understand, you've applied this patch during a merge and no
> further actions are required, correct?
Dave Miller will need to apply that patch (or something similar) when
he merges the wireless-drivers-next tree into the net-next tree. I
will keep applying the patch each day until then.
--
Cheers,
Stephen Rothwell
^ permalink raw reply
* Re: [PATCH v2 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv
From: Alexei Starovoitov @ 2017-06-08 2:49 UTC (permalink / raw)
To: Haishuang Yan
Cc: =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel,
Alexei Starovoitov
In-Reply-To: <1496883476-17445-2-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Thu, Jun 08, 2017 at 08:57:56AM +0800, Haishuang Yan wrote:
> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>
> CC: Alexei Starovoitov <ast@fb.com>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
Good catch. Thanks!
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [RFC PATCH net-next 5/5] selftests/bpf: change test_verifier expectations
From: Alexei Starovoitov via iovisor-dev @ 2017-06-08 2:43 UTC (permalink / raw)
To: Edward Cree
Cc: Alexei Starovoitov, netdev-u79uwXL29TY76Z2rM5mHXA, iovisor-dev,
LKML, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <b04aa1a3-2b6c-2d3d-f26f-17eaaa549539-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
On Wed, Jun 07, 2017 at 04:00:02PM +0100, Edward Cree wrote:
> Some of the verifier's error messages have changed, and some constructs
> that previously couldn't be verified are now accepted.
>
> Signed-off-by: Edward Cree <ecree-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 226 ++++++++++++++--------------
> 1 file changed, 116 insertions(+), 110 deletions(-)
imo this rewrite needs more than one additional test.
Like i counted at least 2 new verifier features (like negative and ptr & 0x40)
All the new logic needs to be covered by tests.
^ permalink raw reply
* Re: linux-next: build failure after merge of the wireless-drivers-next tree
From: Igor Mitsyanko @ 2017-06-08 2:43 UTC (permalink / raw)
To: Stephen Rothwell, Kalle Valo, Wireless, David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List, Dmitrii Lebed,
Sergei Maksimenko, Sergey Matyukevich, Bindu Therthala,
Huizhao Wang, Kamlesh Rath, Avinash Patil
In-Reply-To: <20170608122759.7ffda0c3@canb.auug.org.au>
On 06/07/2017 07:27 PM, Stephen Rothwell wrote:
>
> External Email
>
>
> Hi all,
>
> After merging the wireless-drivers-next tree, today's linux-next build
> (x86_64 allmodconfig) failed like this:
>
> drivers/net/wireless/quantenna/qtnfmac/core.c: In function 'qtnf_core_net_attach':
> drivers/net/wireless/quantenna/qtnfmac/core.c:319:5: error: 'struct net_device' has no member named 'destructor'
> dev->destructor = free_netdev;
> ^
>
> Caused by commit
>
> 98f44cb0655c ("qtnfmac: introduce new FullMAC driver for Quantenna chipsets")
>
> interacting with commit
>
> cf124db566e6 ("net: Fix inconsistent teardown and release of private netdev state.")
>
> from the net tree.
>
> I applied this merge fix patch for today.
Hi Stephen,
thanks. As I understand, you've applied this patch during a merge and no
further actions are required, correct?
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Thu, 8 Jun 2017 12:24:08 +1000
> Subject: [PATCH] qtnfmac: fix up for "net: Fix inconsistent teardown and
> release of private netdev state."
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Acked-by: Igor Mitsyanko <igor.mitsyanko.os@quantenna.com>
> ---
> drivers/net/wireless/quantenna/qtnfmac/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/quantenna/qtnfmac/core.c b/drivers/net/wireless/quantenna/qtnfmac/core.c
> index c5ac252464f4..f053532c0e87 100644
> --- a/drivers/net/wireless/quantenna/qtnfmac/core.c
> +++ b/drivers/net/wireless/quantenna/qtnfmac/core.c
> @@ -316,7 +316,7 @@ int qtnf_core_net_attach(struct qtnf_wmac *mac, struct qtnf_vif *vif,
> vif->netdev = dev;
>
> dev->netdev_ops = &qtnf_netdev_ops;
> - dev->destructor = free_netdev;
> + dev->needs_free_netdev = true;
> dev_net_set(dev, wiphy_net(wiphy));
> dev->ieee80211_ptr = &vif->wdev;
> dev->ieee80211_ptr->iftype = iftype;
> --
> 2.11.0
>
> --
> Cheers,
> Stephen Rothwell
>
^ permalink raw reply
* Re: [RFC PATCH net-next 4/5] bpf/verifier: track signed and unsigned min/max values
From: Alexei Starovoitov via iovisor-dev @ 2017-06-08 2:40 UTC (permalink / raw)
To: Edward Cree
Cc: Alexei Starovoitov, netdev-u79uwXL29TY76Z2rM5mHXA, iovisor-dev,
LKML, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <56b924eb-e2d5-b2ee-484a-d073a3b13d79-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
On Wed, Jun 07, 2017 at 03:59:25PM +0100, Edward Cree wrote:
> Allows us to, sometimes, combine information from a signed check of one
> bound and an unsigned check of the other.
> We now track the full range of possible values, rather than restricting
> ourselves to [0, 1<<30) and considering anything beyond that as
> unknown. While this is probably not necessary, it makes the code more
> straightforward and symmetrical between signed and unsigned bounds.
>
> Signed-off-by: Edward Cree <ecree-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
> ---
> include/linux/bpf_verifier.h | 22 +-
> kernel/bpf/verifier.c | 661 +++++++++++++++++++++++++------------------
> 2 files changed, 395 insertions(+), 288 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index e341469..10a5944 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -11,11 +11,15 @@
> #include <linux/filter.h> /* for MAX_BPF_STACK */
> #include <linux/tnum.h>
>
> - /* Just some arbitrary values so we can safely do math without overflowing and
> - * are obviously wrong for any sort of memory access.
> - */
> -#define BPF_REGISTER_MAX_RANGE (1024 * 1024 * 1024)
> -#define BPF_REGISTER_MIN_RANGE -1
> +/* Maximum variable offset umax_value permitted when resolving memory accesses.
> + * In practice this is far bigger than any realistic pointer offset; this limit
> + * ensures that umax_value + (int)off + (int)size cannot overflow a u64.
> + */
> +#define BPF_MAX_VAR_OFF (1ULL << 31)
> +/* Maximum variable size permitted for ARG_CONST_SIZE[_OR_ZERO]. This ensures
> + * that converting umax_value to int cannot overflow.
> + */
> +#define BPF_MAX_VAR_SIZ INT_MAX
>
> struct bpf_reg_state {
> enum bpf_reg_type type;
> @@ -38,7 +42,7 @@ struct bpf_reg_state {
> * PTR_TO_MAP_VALUE_OR_NULL, we have to NULL-check it _first_.
> */
> u32 id;
> - /* These three fields must be last. See states_equal() */
> + /* These five fields must be last. See states_equal() */
> /* For scalar types (SCALAR_VALUE), this represents our knowledge of
> * the actual value.
> * For pointer types, this represents the variable part of the offset
> @@ -51,8 +55,10 @@ struct bpf_reg_state {
> * These refer to the same value as align, not necessarily the actual
> * contents of the register.
> */
> - s64 min_value; /* minimum possible (s64)value */
> - u64 max_value; /* maximum possible (u64)value */
> + s64 smin_value; /* minimum possible (s64)value */
> + s64 smax_value; /* maximum possible (s64)value */
> + u64 umin_value; /* minimum possible (u64)value */
> + u64 umax_value; /* maximum possible (u64)value */
have uneasy feeling about this one.
It's 16 extra bytes to be stored in every reg_state and memcmp later
while we didn't have cases where people wanted negative values
in ptr+var cases. Why bother than?
> unknown. While this is probably not necessary, it makes the code more
> straightforward and symmetrical between signed and unsigned bounds.
it's hard for me to see the 'straightforward' part yet.
^ permalink raw reply
* Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path
From: Alexei Starovoitov via iovisor-dev @ 2017-06-08 2:35 UTC (permalink / raw)
To: Edward Cree
Cc: Alexei Starovoitov, netdev-u79uwXL29TY76Z2rM5mHXA, iovisor-dev,
LKML, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <47ecf6ca-ae89-7fc3-3cd5-a47009b6ede9-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
On Wed, Jun 07, 2017 at 03:58:50PM +0100, Edward Cree wrote:
> If pointer leaks are allowed, and adjust_ptr_min_max_vals returns -EACCES,
> treat the pointer as an unknown scalar and try again, because we might be
> able to conclude something about the result (e.g. pointer & 0x40 is either
> 0 or 0x40).
>
> Signed-off-by: Edward Cree <ecree-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
> ---
> kernel/bpf/verifier.c | 244 ++++++++++++++++++++++++++------------------------
> 1 file changed, 127 insertions(+), 117 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index dd06e4e..1ff5b5d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1566,6 +1566,8 @@ static void coerce_reg_to_32(struct bpf_reg_state *reg)
> /* Handles arithmetic on a pointer and a scalar: computes new min/max and align.
> * Caller must check_reg_overflow all argument regs beforehand.
> * Caller should also handle BPF_MOV case separately.
> + * If we return -EACCES, caller may want to try again treating pointer as a
> + * scalar. So we only emit a diagnostic if !env->allow_ptr_leaks.
> */
> static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> struct bpf_insn *insn,
> @@ -1588,43 +1590,29 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
>
> if (BPF_CLASS(insn->code) != BPF_ALU64) {
> /* 32-bit ALU ops on pointers produce (meaningless) scalars */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d 32-bit pointer arithmetic prohibited\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - /* High bits are known zero */
> - dst_reg->align.mask = (u32)-1;
> - return 0;
> + return -EACCES;
> }
>
> if (ptr_reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - return 0;
> + return -EACCES;
> }
> if (ptr_reg->type == CONST_PTR_TO_MAP) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic on CONST_PTR_TO_MAP prohibited\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - return 0;
> + return -EACCES;
> }
> if (ptr_reg->type == PTR_TO_PACKET_END) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic on PTR_TO_PACKET_END prohibited\n",
> dst);
> - return -EACCES;
> - }
> - __mark_reg_unknown(dst_reg);
> - return 0;
> + return -EACCES;
> }
>
> /* In case of 'scalar += pointer', dst_reg inherits pointer type and id.
> @@ -1648,8 +1636,9 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> break;
> }
> if (max_val == BPF_REGISTER_MAX_RANGE) {
> - verbose("R%d tried to add unbounded value to pointer\n",
> - dst);
> + if (!env->allow_ptr_leaks)
> + verbose("R%d tried to add unbounded value to pointer\n",
> + dst);
> return -EACCES;
> }
> /* A new variable offset is created. Note that off_reg->off
> @@ -1676,28 +1665,20 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> case BPF_SUB:
> if (dst_reg == off_reg) {
> /* scalar -= pointer. Creates an unknown scalar */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d tried to subtract pointer from scalar\n",
> dst);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> - break;
> + return -EACCES;
> }
> /* We don't allow subtraction from FP, because (according to
> * test_verifier.c test "invalid fp arithmetic", JITs might not
> * be able to deal with it.
> */
> if (ptr_reg->type == PTR_TO_STACK) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d subtraction from stack pointer prohibited\n",
> dst);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> - break;
> + return -EACCES;
> }
> if (known && (ptr_reg->off - min_val ==
> (s64)(s32)(ptr_reg->off - min_val))) {
> @@ -1713,14 +1694,10 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> * This can happen if off_reg is an immediate.
> */
> if ((s64)max_val < 0) {
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d tried to subtract negative max_val %lld from pointer\n",
> dst, (s64)max_val);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> - break;
> + return -EACCES;
> }
> /* A new variable offset is created. If the subtrahend is known
> * nonnegative, then any reg->range we had before is still good.
> @@ -1747,99 +1724,37 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> * (However, in principle we could allow some cases, e.g.
> * ptr &= ~3 which would reduce min_value by 3.)
> */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d bitwise operator %s on pointer prohibited\n",
> dst, bpf_alu_string[opcode >> 4]);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> + return -EACCES;
> default:
> /* other operators (e.g. MUL,LSH) produce non-pointer results */
> - if (!env->allow_ptr_leaks) {
> + if (!env->allow_ptr_leaks)
> verbose("R%d pointer arithmetic with %s operator prohibited\n",
> dst, bpf_alu_string[opcode >> 4]);
> - return -EACCES;
> - }
> - /* Make it an unknown scalar */
> - __mark_reg_unknown(dst_reg);
> + return -EACCES;
> }
>
> check_reg_overflow(dst_reg);
> return 0;
> }
>
> -/* Handles ALU ops other than BPF_END, BPF_NEG and BPF_MOV: computes new min/max
> - * and align.
> - * TODO: check this is legit for ALU32, particularly around negatives
> - */
> -static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
> - struct bpf_insn *insn)
> +static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
> + struct bpf_insn *insn,
> + struct bpf_reg_state *dst_reg,
> + struct bpf_reg_state *src_reg)
> {
> - struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg, *src_reg;
> - struct bpf_reg_state *ptr_reg = NULL, off_reg = {0};
> + struct bpf_reg_state *regs = env->cur_state.regs;
> s64 min_val = BPF_REGISTER_MIN_RANGE;
> u64 max_val = BPF_REGISTER_MAX_RANGE;
> u8 opcode = BPF_OP(insn->code);
> bool src_known, dst_known;
>
> - dst_reg = ®s[insn->dst_reg];
> - check_reg_overflow(dst_reg);
> - src_reg = NULL;
> - if (dst_reg->type != SCALAR_VALUE)
> - ptr_reg = dst_reg;
> - if (BPF_SRC(insn->code) == BPF_X) {
> - src_reg = ®s[insn->src_reg];
> - check_reg_overflow(src_reg);
> -
> - if (src_reg->type != SCALAR_VALUE) {
> - if (dst_reg->type != SCALAR_VALUE) {
> - /* Combining two pointers by any ALU op yields
> - * an arbitrary scalar.
> - */
> - if (!env->allow_ptr_leaks) {
> - verbose("R%d pointer %s pointer prohibited\n",
> - insn->dst_reg,
> - bpf_alu_string[opcode >> 4]);
> - return -EACCES;
> - }
> - mark_reg_unknown(regs, insn->dst_reg);
> - return 0;
> - } else {
> - /* scalar += pointer
> - * This is legal, but we have to reverse our
> - * src/dest handling in computing the range
> - */
> - return adjust_ptr_min_max_vals(env, insn,
> - src_reg, dst_reg);
> - }
> - } else if (ptr_reg) {
> - /* pointer += scalar */
> - return adjust_ptr_min_max_vals(env, insn,
> - dst_reg, src_reg);
> - }
> - } else {
> - /* Pretend the src is a reg with a known value, since we only
> - * need to be able to read from this state.
> - */
> - off_reg.type = SCALAR_VALUE;
> - off_reg.align = tn_const(insn->imm);
> - off_reg.min_value = insn->imm;
> - off_reg.max_value = insn->imm;
> - src_reg = &off_reg;
> - if (ptr_reg) /* pointer += K */
> - return adjust_ptr_min_max_vals(env, insn,
> - ptr_reg, src_reg);
> - }
> -
> - /* Got here implies adding two SCALAR_VALUEs */
> - if (WARN_ON_ONCE(ptr_reg)) {
> - verbose("verifier internal error\n");
> - return -EINVAL;
> - }
> - if (WARN_ON(!src_reg)) {
> - verbose("verifier internal error\n");
> - return -EINVAL;
such large back and forth move doesn't help reviewing.
may be just merge it into previous patch?
Or keep that function in the right place in patch 2 already?
^ permalink raw reply
* Re: [RFC PATCH net-next 2/5] bpf/verifier: rework value tracking
From: Alexei Starovoitov via iovisor-dev @ 2017-06-08 2:32 UTC (permalink / raw)
To: Edward Cree
Cc: Alexei Starovoitov, netdev-u79uwXL29TY76Z2rM5mHXA, iovisor-dev,
LKML, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <cef78266-62ec-8ef7-a512-cc111ba9c22a-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
On Wed, Jun 07, 2017 at 03:58:31PM +0100, Edward Cree wrote:
> Tracks value alignment by means of tracking known & unknown bits.
> Tightens some min/max value checks and fixes a couple of bugs therein.
>
> Signed-off-by: Edward Cree <ecree-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
> ---
> include/linux/bpf.h | 34 +-
> include/linux/bpf_verifier.h | 40 +-
> include/linux/tnum.h | 58 ++
> kernel/bpf/Makefile | 2 +-
> kernel/bpf/tnum.c | 163 +++++
> kernel/bpf/verifier.c | 1641 +++++++++++++++++++++++-------------------
> 6 files changed, 1170 insertions(+), 768 deletions(-)
yeah! That's cool. Overall I like the direction.
I don't understand it completely yet, so ony few nits so far:
> +/* Arithmetic and logical ops */
> +/* Shift a tnum left (by a fixed shift) */
> +struct tnum tn_sl(struct tnum a, u8 shift);
> +/* Shift a tnum right (by a fixed shift) */
> +struct tnum tn_sr(struct tnum a, u8 shift);
I think in few month we will forget what these abbreviations mean.
Can you change it to tnum_rshift, tnum_lshift, tnum_add ?
> +/* half-multiply add: acc += (unknown * mask * value) */
> +static struct tnum hma(struct tnum acc, u64 value, u64 mask)
hma? is it a standard abbreviation?
> -static void init_reg_state(struct bpf_reg_state *regs)
> +static void mark_reg_known_zero(struct bpf_reg_state *regs, u32 regno)
> {
> - int i;
> -
> - for (i = 0; i < MAX_BPF_REG; i++)
> - mark_reg_not_init(regs, i);
> -
> - /* frame pointer */
> - regs[BPF_REG_FP].type = FRAME_PTR;
> -
> - /* 1st arg to a function */
> - regs[BPF_REG_1].type = PTR_TO_CTX;
> + BUG_ON(regno >= MAX_BPF_REG);
> + __mark_reg_known_zero(regs + regno);
I know we have BUG_ONs in the code and it was never hit,
but since you're rewriting it please change it to WARN_ON and
set all regs into NOT_INIT in such case.
This way if we really have a bug, it hopefully won't crash.
> -/* check read/write into an adjusted map element */
> -static int check_map_access_adj(struct bpf_verifier_env *env, u32 regno,
> +/* check read/write into a map element with possible variable offset */
> +static int check_map_access(struct bpf_verifier_env *env, u32 regno,
> int off, int size)
> {
> struct bpf_verifier_state *state = &env->cur_state;
> struct bpf_reg_state *reg = &state->regs[regno];
> int err;
>
> - /* We adjusted the register to this map value, so we
> - * need to change off and size to min_value and max_value
> - * respectively to make sure our theoretical access will be
> - * safe.
> + /* We may have adjusted the register to this map value, so we
> + * need to try adding each of min_value and max_value to off
> + * to make sure our theoretical access will be safe.
> */
> if (log_level)
> print_verifier_state(state);
> - env->varlen_map_value_access = true;
> + /* If the offset is variable, we will need to be stricter in state
> + * pruning from now on.
> + */
> + if (reg->align.mask)
> + env->varlen_map_value_access = true;
i think this align.mask access was used in few places.
May be worth to do static inline helper with clear name?
> switch (reg->type) {
> case PTR_TO_PACKET:
> + /* special case, because of NET_IP_ALIGN */
> return check_pkt_ptr_alignment(reg, off, size, strict);
> - case PTR_TO_MAP_VALUE_ADJ:
> - return check_val_ptr_alignment(reg, size, strict);
> + case PTR_TO_MAP_VALUE:
> + pointer_desc = "value ";
> + break;
> + case PTR_TO_CTX:
> + pointer_desc = "context ";
> + break;
> + case PTR_TO_STACK:
> + pointer_desc = "stack ";
> + break;
thank you for making errors more human readable.
> + char tn_buf[48];
> +
> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
> + verbose("variable ctx access align=%s off=%d size=%d",
> + tn_buf, off, size);
> + return -EACCES;
> + }
> + off += reg->align.value;
I think 'align' is an odd name for this field.
May be rename off/align fields into
s32 fixed_off;
struct tnum var_off;
>
> - } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
> + } else if (reg->type == PTR_TO_STACK) {
> + /* stack accesses must be at a fixed offset, so that we can
> + * determine what type of data were returned.
> + */
> + if (reg->align.mask) {
> + char tn_buf[48];
> +
> + tn_strn(tn_buf, sizeof(tn_buf), reg->align);
> + verbose("variable stack access align=%s off=%d size=%d",
> + tn_buf, off, size);
> + return -EACCES;
hmm. why this restriction?
I thought one of key points of the diff that ptr+var tracking logic
will now apply not only to map_value, but to stack_ptr as well?
> }
>
> - if (!err && size <= 2 && value_regno >= 0 && env->allow_ptr_leaks &&
> - state->regs[value_regno].type == UNKNOWN_VALUE) {
> - /* 1 or 2 byte load zero-extends, determine the number of
> - * zero upper bits. Not doing it fo 4 byte load, since
> - * such values cannot be added to ptr_to_packet anyway.
> - */
> - state->regs[value_regno].imm = 64 - size * 8;
> + if (!err && size < BPF_REG_SIZE && value_regno >= 0 && t == BPF_READ &&
> + state->regs[value_regno].type == SCALAR_VALUE) {
> + /* b/h/w load zero-extends, mark upper bits as known 0 */
> + state->regs[value_regno].align.value &= (1ULL << (size * 8)) - 1;
> + state->regs[value_regno].align.mask &= (1ULL << (size * 8)) - 1;
probably another helper from tnum.h is needed.
> + /* sign bit is known zero, so we can bound the value */
> + state->regs[value_regno].min_value = 0;
> + state->regs[value_regno].max_value = min_t(u64,
> + state->regs[value_regno].align.mask,
> + BPF_REGISTER_MAX_RANGE);
min_t with mask? should it be align.value?
> }
> return err;
> }
> @@ -1000,9 +1068,18 @@ static int check_xadd(struct bpf_verifier_env *env, struct bpf_insn *insn)
> BPF_SIZE(insn->code), BPF_WRITE, -1);
> }
>
> +/* Does this register contain a constant zero? */
> +static bool register_is_null(struct bpf_reg_state reg)
> +{
> + return reg.type == SCALAR_VALUE && reg.align.mask == 0 &&
> + reg.align.value == 0;
align.mask == 0 && align.value==0 into helper in tnum.h ?
> @@ -1024,7 +1101,15 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
> return -EACCES;
> }
>
> - off = regs[regno].imm;
> + /* Only allow fixed-offset stack reads */
> + if (regs[regno].align.mask) {
> + char tn_buf[48];
> +
> + tn_strn(tn_buf, sizeof(tn_buf), regs[regno].align);
> + verbose("invalid variable stack read R%d align=%s\n",
> + regno, tn_buf);
> + }
same question as before. can it be relaxed?
The support for char arr[32]; accee arr[n] was requested several times
and folks used map_value[n] as a workaround.
Seems with this var stack logic it's one step away, no?
> - if (src_reg->imm < 48) {
> - verbose("cannot add integer value with %lld upper zero bits to ptr_to_packet\n",
> - src_reg->imm);
> - return -EACCES;
> - }
> -
> - had_id = (dst_reg->id != 0);
> -
> - /* dst_reg stays as pkt_ptr type and since some positive
> - * integer value was added to the pointer, increment its 'id'
> - */
> - dst_reg->id = ++env->id_gen;
great to see it's being generalized.
> + if (ptr_reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
> + if (!env->allow_ptr_leaks) {
> + verbose("R%d pointer arithmetic on PTR_TO_MAP_VALUE_OR_NULL prohibited, null-check it first\n",
> + dst);
> + return -EACCES;
> + }
i guess mark_map_reg() logic will cover good cases and
actual math on ptr_to_map_or_null will happen only in broken programs.
just feels a bit fragile, since it probably depends on order we will
evaluate the branches? it's not an issue with this patch. we have
the same situation today. just thinking out loud.
> + /* Got here implies adding two SCALAR_VALUEs */
> + if (WARN_ON_ONCE(ptr_reg)) {
> + verbose("verifier internal error\n");
> + return -EINVAL;
...
> + if (WARN_ON(!src_reg)) {
> + verbose("verifier internal error\n");
> + return -EINVAL;
> }
i'm lost with these bits.
Can you add a comment in what circumstances this can be hit
and what would be the consequences?
> +/* Returns true if (rold safe implies rcur safe) */
> +static bool regsafe(struct bpf_reg_state *rold,
> + struct bpf_reg_state *rcur,
> + bool varlen_map_access)
> +{
> + if (memcmp(rold, rcur, sizeof(*rold)) == 0)
> return true;
> + if (rold->type == NOT_INIT)
> + /* explored state can't have used this */
> return true;
> + if (rcur->type == NOT_INIT)
> + return false;
> + switch (rold->type) {
> + case SCALAR_VALUE:
> + if (rcur->type == SCALAR_VALUE) {
> + /* new val must satisfy old val knowledge */
> + return range_within(rold, rcur) &&
> + tn_in(rold->align, rcur->align);
> + } else {
> + /* if we knew anything about the old value, we're not
> + * equal, because we can't know anything about the
> + * scalar value of the pointer in the new value.
> + */
> + return rold->min_value == BPF_REGISTER_MIN_RANGE &&
> + rold->max_value == BPF_REGISTER_MAX_RANGE &&
> + !~rold->align.mask;
> + }
> + case PTR_TO_MAP_VALUE:
> + if (varlen_map_access) {
> + /* If the new min/max/align satisfy the old ones and
> + * everything else matches, we are OK.
> + * We don't care about the 'id' value, because nothing
> + * uses it for PTR_TO_MAP_VALUE (only for ..._OR_NULL)
> + */
> + return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
> + range_within(rold, rcur) &&
> + tn_in(rold->align, rcur->align);
> + } else {
> + /* If the ranges/align were not the same, but
> + * everything else was and we didn't do a variable
> + * access into a map then we are a-ok.
> + */
> + return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0;
> + }
> + case PTR_TO_MAP_VALUE_OR_NULL:
does this new state comparison logic helps?
Do you have any numbers before/after in the number of insns it had to process
for the tests in selftests ?
^ permalink raw reply
* linux-next: build failure after merge of the wireless-drivers-next tree
From: Stephen Rothwell @ 2017-06-08 2:27 UTC (permalink / raw)
To: Kalle Valo, Wireless, David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
Igor Mitsyanko, Dmitrii Lebed, Sergei Maksimenko,
Sergey Matyukevich, Bindu Therthala, Huizhao Wang, Kamlesh Rath,
Avinash Patil
Hi all,
After merging the wireless-drivers-next tree, today's linux-next build
(x86_64 allmodconfig) failed like this:
drivers/net/wireless/quantenna/qtnfmac/core.c: In function 'qtnf_core_net_attach':
drivers/net/wireless/quantenna/qtnfmac/core.c:319:5: error: 'struct net_device' has no member named 'destructor'
dev->destructor = free_netdev;
^
Caused by commit
98f44cb0655c ("qtnfmac: introduce new FullMAC driver for Quantenna chipsets")
interacting with commit
cf124db566e6 ("net: Fix inconsistent teardown and release of private netdev state.")
from the net tree.
I applied this merge fix patch for today.
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 8 Jun 2017 12:24:08 +1000
Subject: [PATCH] qtnfmac: fix up for "net: Fix inconsistent teardown and
release of private netdev state."
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/net/wireless/quantenna/qtnfmac/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/quantenna/qtnfmac/core.c b/drivers/net/wireless/quantenna/qtnfmac/core.c
index c5ac252464f4..f053532c0e87 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/core.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/core.c
@@ -316,7 +316,7 @@ int qtnf_core_net_attach(struct qtnf_wmac *mac, struct qtnf_vif *vif,
vif->netdev = dev;
dev->netdev_ops = &qtnf_netdev_ops;
- dev->destructor = free_netdev;
+ dev->needs_free_netdev = true;
dev_net_set(dev, wiphy_net(wiphy));
dev->ieee80211_ptr = &vif->wdev;
dev->ieee80211_ptr->iftype = iftype;
--
2.11.0
--
Cheers,
Stephen Rothwell
^ permalink raw reply related
* Re: [PATCH v2 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv
From: Eric Dumazet @ 2017-06-08 2:23 UTC (permalink / raw)
To: Haishuang Yan; +Cc: netdev
In-Reply-To: <1496883476-17445-2-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Thu, 2017-06-08 at 08:57 +0800, Haishuang Yan wrote:
> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>
> CC: Alexei Starovoitov <ast@fb.com>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> ---
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply
* Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv
From: Eric Dumazet @ 2017-06-08 2:21 UTC (permalink / raw)
To: Haishuang Yan; +Cc: netdev, linux-kernel, Pravin B Shelar, Haishuang Yan
In-Reply-To: <1496883476-17445-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Thu, 2017-06-08 at 08:57 +0800, Haishuang Yan wrote:
> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
> ---
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks a lot.
^ permalink raw reply
* Re: [PATCH v2 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv
From: Pravin Shelar @ 2017-06-08 2:13 UTC (permalink / raw)
To: Haishuang Yan
Cc: =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy,
Linux Kernel Network Developers, linux-kernel, Pravin B Shelar,
Haishuang Yan
In-Reply-To: <1496883476-17445-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Wed, Jun 7, 2017 at 5:57 PM, Haishuang Yan
<yanhaishuang@cmss.chinamobile.com> wrote:
> When ip_tunnel_rcv fails, the tun_dst won't be freed, so move
> skb_dst_set to begin and tun_dst would be freed by kfree_skb.
>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com390e>
> ---
> net/ipv4/ip_tunnel.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index b878ecb..27fc20f 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -386,6 +386,9 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
> const struct iphdr *iph = ip_hdr(skb);
> int err;
>
> + if (tun_dst)
> + skb_dst_set(skb, (struct dst_entry *)tun_dst);
> +
If dst is set so early, skb_scrub_packet() would remove the tunnel dst
reference.
It is better to call skb_dst_drop() from error code path.
> #ifdef CONFIG_NET_IPGRE_BROADCAST
> if (ipv4_is_multicast(iph->daddr)) {
> tunnel->dev->stats.multicast++;
> @@ -439,9 +442,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
> skb->dev = tunnel->dev;
> }
>
> - if (tun_dst)
> - skb_dst_set(skb, (struct dst_entry *)tun_dst);
> -
> gro_cells_receive(&tunnel->gro_cells, skb);
> return 0;
>
> --
> 1.8.3.1
>
>
>
^ permalink raw reply
* Re: [PATCH 2/2(net.git)] stmmac: fix for hw timestamp of GMAC3 unit
From: Andy Shevchenko @ 2017-06-08 1:31 UTC (permalink / raw)
To: Mario Molitor
Cc: Giuseppe CAVALLARO, Alexandre TORGUE, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <trinity-988824f2-9aed-4ed0-bc97-d78b13337cf5-1496825121649@3capp-webde-bs41>
On Wed, Jun 7, 2017 at 11:45 AM, Mario Molitor <Mario_Molitor@web.de> wrote:
> exit:
> - return ret;
> + if (likely(ret == 0))
> + return 1;
> + else
> + return 0;
else is redundant.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers
From: Vivien Didelot @ 2017-06-08 1:03 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, Andrew Lunn, John Crispin, David Miller, Sean Wang
In-Reply-To: <a4da565e-7339-5501-205f-540941059d51@gmail.com>
Hi Florian,
Florian Fainelli <f.fainelli@gmail.com> writes:
>> Also I do not think that it is a good thing that a DSA driver play much
>> in dsa_port structures (they are ideally DSA core only specific data).
>> They only seem to need the master interface, so what I see is:
>>
>> static inline struct net_device *dsa_get_master(struct dsa_switch *ds, int port)
>> {
>> struct dsa_port *dp = &ds->ports[port];
>>
>> if (!dsa_is_cpu_port(ds, port))
>> dp = dp->cpu_dp;
>>
>> return dp->netdev;
>> }
>
> The port parameter is kind of pointless, that is what I was trying to
> say, see below.
>
> MT7530_CPU_PORT = 6 and there is a define above for 7 ports, so it is
> presumably the default CPU port that the switch uses. With Broadcom
> switches you could have port 5, 7 or 8 as CPU ports but 8 is still the
> default for most 9-ports capable switches.
>
>> Now let's think that you can have several CPU ports (as with mv88e6xxx).
>> I think it is the driver responsibility to iterate over CPU capable
>> ports and inspect the master devices if they need to, instead of having
>> DSA core return an arbitrary one (which might be the wrong one.)
>
> OK, then are you okay if I do this in mt7530.c instead:
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 1e46418a3b74..d5b63958dd85 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -907,17 +907,26 @@ static int
> mt7530_setup(struct dsa_switch *ds)
> {
> struct mt7530_priv *priv = ds->priv;
> + struct dsa_port *port;
> int ret, i;
> u32 id, val;
> struct device_node *dn;
> struct mt7530_dummy_poll p;
>
> - /* The parent node of cpu_dp->netdev which holds the common system
> - * controller also is the container for two GMACs nodes representing
> - * as two netdev instances.
> - */
> - dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
> - priv->ethernet = syscon_node_to_regmap(dn);
> + for (i = 0; i < ds->num_ports; i++) {
> + port = &ds->ports[i];
> + /* The parent node of cpu_dp->netdev which holds the common
> + * system controller also is the container for two GMACs
> nodes
> + * representing as two netdev instances.
> + */
> + if (dsa_is_cpu_port(ds, i)) {
> + dn = port->netdev->dev.of_node->parent;
> + if (priv->ethernet)
> + return -EEXIST;
> + priv->ethernet = syscon_node_to_regmap(dn);
> + }
> + }
> +
> if (IS_ERR(priv->ethernet))
> return PTR_ERR(priv->ethernet);
I think it is the correct way to do this.
This was the reason why I suggested a dsa_get_master() alternative, even
though it is not necessary per-se, it was just to reduce boilerplate and
avoid exposing too much the dsa_port structures.
dsa_ds_get_cpu_dp() doesn't make much sense anymore, right? If there are
2 CPU ports in use, it is better that the driver iterates on them itself
as you did, instead of having DSA return the first one it knows about.
Thanks,
Vivien
^ permalink raw reply
* [PATCH v2 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv
From: Haishuang Yan @ 2017-06-08 0:57 UTC (permalink / raw)
To: =David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, linux-kernel, Haishuang Yan, Alexei Starovoitov
In-Reply-To: <1496883476-17445-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
When __ip6_tnl_rcv fails, the tun_dst won't be freed, so move
skb_dst_set to begin and tun_dst would be freed by kfree_skb.
CC: Alexei Starovoitov <ast@fb.com>
Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
net/ipv6/ip6_tunnel.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 9b37f97..bf45f1b 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -789,6 +789,9 @@ static int __ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
int err;
+ if (tun_dst)
+ skb_dst_set(skb, (struct dst_entry *)tun_dst);
+
if ((!(tpi->flags & TUNNEL_CSUM) &&
(tunnel->parms.i_flags & TUNNEL_CSUM)) ||
((tpi->flags & TUNNEL_CSUM) &&
@@ -852,9 +855,6 @@ static int __ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
skb_scrub_packet(skb, !net_eq(tunnel->net, dev_net(tunnel->dev)));
- if (tun_dst)
- skb_dst_set(skb, (struct dst_entry *)tun_dst);
-
gro_cells_receive(&tunnel->gro_cells, skb);
return 0;
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox