Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
From: Andrew Lunn @ 2018-09-27 13:25 UTC (permalink / raw)
  To: Yangbo Lu
  Cc: linux-kernel, devel, netdev, Richard Cochran, David S . Miller,
	Ioana Radulescu, Greg Kroah-Hartman
In-Reply-To: <20180927111228.46118-1-yangbo.lu@nxp.com>

On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> This patch is to move DPAA2 PTP driver out of staging/
> since the dpaa2-eth had been moved out.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
>  drivers/net/ethernet/freescale/Kconfig             |    9 +--------
>  drivers/net/ethernet/freescale/dpaa2/Kconfig       |   15 +++++++++++++++
>  drivers/net/ethernet/freescale/dpaa2/Makefile      |    6 ++++--
>  .../ethernet/freescale/dpaa2}/dprtc-cmd.h          |    0
>  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c   |    0
>  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h   |    0
>  .../rtc => net/ethernet/freescale/dpaa2}/rtc.c     |    0
>  .../rtc => net/ethernet/freescale/dpaa2}/rtc.h     |    0
>  drivers/staging/fsl-dpaa2/Kconfig                  |    8 --------
>  drivers/staging/fsl-dpaa2/Makefile                 |    1 -
>  drivers/staging/fsl-dpaa2/rtc/Makefile             |    7 -------
>  11 files changed, 20 insertions(+), 26 deletions(-)
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/dprtc.c (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/dprtc.h (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.h (100%)

Hi Yangbo

Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the
name, change it to ptp.[ch]. Also, some of the function names, and
structures, rtc_probe->ptp_probe, rtc_remove->ptp_remove,
rtc_match_id_table-> ptp_match_id_table, etc.

ptp_dpaa2_adjfreq() probably should return err, not 0.
ptp_dpaa2_gettime() again does not return the error.
If fact, it seems like all the main functions ignore errors.

kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
Can ptp_dpaa2_caps be made const?
dpaa2_phc_index does not appear to be used.
dev_set_drvdata(dev, NULL); is not needed.
Can rtc_drv be made const?
Is rtc.h used by anything other than rtc.c? It seems like it can be removed.

It seems like there is a lot of code in dprtc.c which is unused. rtc.c
does nothing with interrupts for example. Do you plan to make use of
this extra code? Or can it be removed leaving just what is needed?

struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct
seems very odd. And it is not the only example.

      Andrew

^ permalink raw reply

* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Jason A. Donenfeld @ 2018-09-27 13:26 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ard Biesheuvel, LKML, Netdev, Linux Crypto Mailing List,
	David Miller, Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
	Jean-Philippe Aumasson, Russell King - ARM Linux,
	linux-arm-kernel
In-Reply-To: <CAHmME9qNn5aRgtbV3bAsz3xW1A49a7RMMkOzGruBUPzLVUxVNg@mail.gmail.com>

Hi Thomas,

I'm trying to optimize this for crypto performance while still taking
into account preemption concerns. I'm having a bit of trouble figuring
out a way to determine numerically what the upper bounds for this
stuff looks like. I'm sure I could pick a pretty sane number that's
arguably okay -- and way under the limit -- but I still am interested
in determining what that limit actually is. I was hoping there'd be a
debugging option called, "warn if preemption is disabled for too
long", or something, but I couldn't find anything like that. I'm also
not quite sure what the latency limits are, to just compute this with
a formula. Essentially what I'm trying to determine is:

preempt_disable();
asm volatile(".fill N, 1, 0x90;");
preempt_enable();

What is the maximum value of N for which the above is okay? What
technique would you generally use in measuring this?

Thanks,
Jason

^ permalink raw reply

* [PATCH bpf-next] bpf: test_bpf: add dummy dev->net for flow_dissector
From: Song Liu @ 2018-09-27  7:15 UTC (permalink / raw)
  To: netdev; +Cc: daniel, kernel-team, Song Liu, Willem de Bruijn, Petar Penkov

Latest changes in __skb_flow_dissect() assume skb->dev has valid nd_net.
However, this is not true for test_bpf. As a result, test_bpf.ko crashes
the system with the following stack trace:

[ 1133.716622] BUG: unable to handle kernel paging request at 0000000000001030
[ 1133.716623] PGD 8000001fbf7ee067
[ 1133.716624] P4D 8000001fbf7ee067
[ 1133.716624] PUD 1f6c1cf067
[ 1133.716625] PMD 0
[ 1133.716628] Oops: 0000 [#1] SMP PTI
[ 1133.716630] CPU: 7 PID: 40473 Comm: modprobe Kdump: loaded Not tainted 4.19.0-rc5-00805-gca11cc92ccd2 #1167
[ 1133.716631] Hardware name: Wiwynn Leopard-Orv2/Leopard-DDR BW, BIOS LBM12.5 12/06/2017
[ 1133.716638] RIP: 0010:__skb_flow_dissect+0x83/0x1680
[ 1133.716639] Code: 04 00 00 41 0f b7 44 24 04 48 85 db 4d 8d 14 07 0f 84 01 02 00 00 48 8b 43 10 48 85 c0 0f 84 e5 01 00 00 48 8b 80 a8 04 00 00 <48> 8b 90 30 10 00 00 48 85 d2 0f 84 dd 01 00 00 31 c0 b9 05 00 00
[ 1133.716640] RSP: 0018:ffffc900303c7a80 EFLAGS: 00010282
[ 1133.716642] RAX: 0000000000000000 RBX: ffff881fea0b7400 RCX: 0000000000000000
[ 1133.716643] RDX: ffffc900303c7bb4 RSI: ffffffff8235c3e0 RDI: ffff881fea0b7400
[ 1133.716643] RBP: ffffc900303c7b80 R08: 0000000000000000 R09: 000000000000000e
[ 1133.716644] R10: ffffc900303c7bb4 R11: ffff881fb6840400 R12: ffffffff8235c3e0
[ 1133.716645] R13: 0000000000000008 R14: 000000000000001e R15: ffffc900303c7bb4
[ 1133.716646] FS:  00007f54e75d3740(0000) GS:ffff881fff5c0000(0000) knlGS:0000000000000000
[ 1133.716648] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1133.716649] CR2: 0000000000001030 CR3: 0000001f6c226005 CR4: 00000000003606e0
[ 1133.716649] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1133.716650] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1133.716651] Call Trace:
[ 1133.716660]  ? sched_clock_cpu+0xc/0xa0
[ 1133.716662]  ? sched_clock_cpu+0xc/0xa0
[ 1133.716665]  ? log_store+0x1b5/0x260
[ 1133.716667]  ? up+0x12/0x60
[ 1133.716669]  ? skb_get_poff+0x4b/0xa0
[ 1133.716674]  ? __kmalloc_reserve.isra.47+0x2e/0x80
[ 1133.716675]  skb_get_poff+0x4b/0xa0
[ 1133.716680]  bpf_skb_get_pay_offset+0xa/0x10
[ 1133.716686]  ? test_bpf_init+0x578/0x1000 [test_bpf]
[ 1133.716690]  ? netlink_broadcast_filtered+0x153/0x3d0
[ 1133.716695]  ? free_pcppages_bulk+0x324/0x600
[ 1133.716696]  ? 0xffffffffa0279000
[ 1133.716699]  ? do_one_initcall+0x46/0x1bd
[ 1133.716704]  ? kmem_cache_alloc_trace+0x144/0x1a0
[ 1133.716709]  ? do_init_module+0x5b/0x209
[ 1133.716712]  ? load_module+0x2136/0x25d0
[ 1133.716715]  ? __do_sys_finit_module+0xba/0xe0
[ 1133.716717]  ? __do_sys_finit_module+0xba/0xe0
[ 1133.716719]  ? do_syscall_64+0x48/0x100
[ 1133.716724]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9

This patch fixes tes_bpf by adding a dummy struct net to the dummy dev.

Fixes: d0e13a1488ad ("flow_dissector: lookup netns by skb->sk if skb->dev is NULL")
Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
Cc: Willem de Bruijn <willemb@google.com>
Cc: Petar Penkov <ppenkov@google.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
 lib/test_bpf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 08d3d59dca17..5e29d4bd06c8 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -6470,6 +6470,7 @@ static struct bpf_test tests[] = {
 	},
 };
 
+static struct net net;
 static struct net_device dev;
 
 static struct sk_buff *populate_skb(char *buf, int size)
@@ -6494,6 +6495,8 @@ static struct sk_buff *populate_skb(char *buf, int size)
 	skb->queue_mapping = SKB_QUEUE_MAP;
 	skb->vlan_tci = SKB_VLAN_TCI;
 	skb->vlan_proto = htons(ETH_P_IP);
+	dev_net_set(&dev, &net);
+	RCU_INIT_POINTER(net.flow_dissector_prog, NULL);
 	skb->dev = &dev;
 	skb->dev->ifindex = SKB_DEV_IFINDEX;
 	skb->dev->type = SKB_DEV_TYPE;
-- 
2.17.1

^ permalink raw reply related

* Re: [PATCH] netlink: add policy attribute range validation
From: Michal Kubecek @ 2018-09-27  7:16 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, netdev
In-Reply-To: <1537994127.28767.39.camel@sipsolutions.net>

On Wed, Sep 26, 2018 at 10:35:27PM +0200, Johannes Berg wrote:
> On Wed, 2018-09-26 at 22:17 +0200, Johannes Berg wrote:
> > On Wed, 2018-09-26 at 22:06 +0200, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > Without further bloating the policy structs, we can overload
> > > the `validation_data' pointer with a struct of s16 min, max
> > > and use those to validate ranges in NLA_{U,S}{8,16,32,64}
> > > attributes.
> > > 
> > > It may sound strange to validate NLA_U32 with a s16 max, but
> > > in many cases NLA_U32 is used for enums etc. since there's no
> > > size benefit in using a smaller attribute width anyway, due
> > > to netlink attribute alignment; in cases like that it's still
> > > useful, particularly when the attribute really transports an
> > > enum value.
> > 
> > That said, I did find a few places where we could benefit from a larger
> > type here - e.g. having a NLA_U16 that must be non-zero cannot be
> > represented in the policy as is, since you can't set max to 65535.
> 
> We could also fix that, btw, by taking two bits out of the "type" field,
> and letting those indicate "check_min" and "check_max". That would also
> fix the other thing I noted regarding the union, I suppose.
> 
> I didn't really like that too much because it makes the whole thing far
> more complex, but perhaps if we hide it behind macros like
> 
> #define NLA_POLICY_RANGE(tp, _min, _max) {
> 	.type = tp,
> 	.min = _min, .check_min = 1,
> 	.max = _max, .check_max = 1,
> }
> 
> #define NLA_POLICY_MIN(tp, _min) {
> 	.type = tp,
> 	.min = _min, .check_min = 1,
> }
> 
> #define NLA_POLICY_MAX(tp, _max) {
> 	.type = tp,
> 	.max = _max, .check_max = 1,
> }
> 
> it becomes more palatable?

The overloading still feels a bit complicated. Perhaps we could rather
use validation_data in the natural way, i.e. as a pointer to validation
data. That would be a struct (maybe array) of two values of the
corresponding type. It would mean a bit more data and a bit more writing
but it would be IMHO more straightforward.

Michal Kubecek

^ permalink raw reply

* RE: [PATCH v2 00/22] SMMU enablement for NXP LS1043A and LS1046A
From: Madalin-cristian Bucur @ 2018-09-27 13:39 UTC (permalink / raw)
  To: Laurentiu Tudor, davem@davemloft.net
  Cc: devicetree@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, Roy Pledge,
	shawnguo@kernel.org, Leo Li, robin.murphy@arm.com, Bharat Bhushan
In-Reply-To: <20180926132247.10971-1-laurentiu.tudor@nxp.com>

> -----Original Message-----
> From: laurentiu.tudor@nxp.com [mailto:laurentiu.tudor@nxp.com]
> Sent: Wednesday, September 26, 2018 4:22 PM
> To: devicetree@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: Roy Pledge <roy.pledge@nxp.com>; Madalin-cristian Bucur
> <madalin.bucur@nxp.com>; davem@davemloft.net; shawnguo@kernel.org; Leo Li
> <leoyang.li@nxp.com>; robin.murphy@arm.com; Bharat Bhushan
> <bharat.bhushan@nxp.com>; Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Subject: [PATCH v2 00/22] SMMU enablement for NXP LS1043A and LS1046A
> 
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> This patch series adds SMMU support for NXP LS1043A and LS1046A chips
> and consists mostly in important driver fixes and the required device
> tree updates. It touches several subsystems and consists of three main
> parts:
>  - changes in soc/drivers/fsl/qbman drivers adding iommu mapping of
>    reserved memory areas, fixes and defered probe support
>  - changes in drivers/net/ethernet/freescale/dpaa_eth drivers
>    consisting in misc dma mapping related fixes and probe ordering
>  - addition of the actual arm smmu device tree node together with
>    various adjustments to the device trees
> 
> Performance impact
> 
>     Running iperf benchmarks in a back-to-back setup (both sides
>     having smmu enabled) on a 10GBps port show an important
>     networking performance degradation of around 40% (9.48Gbps
>     linerate vs 5.45Gbps) and around 30%-35% with iommu.strict=1.
>     If you need performance but without SMMU support you can use
>     "iommu.passthrough=1" to disable SMMU.
> 
> The patch set is based on net-next so, if generally agreed, I'd suggest
> to get the patches through the netdev tree after getting all the Acks.
> 
> Changes in v2:
>  - dropped confusing comments in smmu interrupt property (Robin)
>  - add an intermediate simple-bus for usb to fix dma size issue (Robin)
>  - use defines instead of numbers in smmu interrupt definition
>  - drop redundant double iommu_get_domain_for_dev() call in few qbman
>    patches
> 
> Laurentiu Tudor (22):
>   soc/fsl/qman: fixup liodns only on ppc targets
>   soc/fsl/bman: map FBPR area in the iommu
>   soc/fsl/qman: map FQD and PFDR areas in the iommu
>   soc/fsl/qman-portal: map CENA area in the iommu
>   soc/fsl/qbman: add APIs to retrieve the probing status
>   soc/fsl/qman_portals: defer probe after qman's probe
>   soc/fsl/bman_portals: defer probe after bman's probe
>   soc/fsl/qbman_portals: add APIs to retrieve the probing status
>   fsl/fman: backup and restore ICID registers
>   fsl/fman: add API to get the device behind a fman port
>   dpaa_eth: defer probing after qbman
>   dpaa_eth: base dma mappings on the fman rx port
>   dpaa_eth: fix iova handling for contiguous frames
>   dpaa_eth: fix iova handling for sg frames
>   dpaa_eth: fix SG frame cleanup
>   arm64: dts: ls1046a: add smmu node
>   arm64: dts: ls1043a: add smmu node
>   arm64: dts: ls104xa: set mask to drop TBU ID from StreamID
>   arm64: dts: ls104x: add missing dma ranges property
>   arm64: dts: ls104x: add iommu-map to pci controllers
>   arm64: dts: ls104x: make dma-coherent global to the SoC
>   arm64: dts: ls104x: use a pseudo-bus to constrain usb dma size
> 
>  .../arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 106 ++++++++++----
>  .../arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 102 ++++++++++---
>  .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 136 ++++++++++++------
>  drivers/net/ethernet/freescale/fman/fman.c    |  35 ++++-
>  drivers/net/ethernet/freescale/fman/fman.h    |   4 +
>  .../net/ethernet/freescale/fman/fman_port.c   |  14 ++
>  .../net/ethernet/freescale/fman/fman_port.h   |   2 +
>  drivers/soc/fsl/qbman/bman_ccsr.c             |  22 +++
>  drivers/soc/fsl/qbman/bman_portal.c           |  20 ++-
>  drivers/soc/fsl/qbman/qman_ccsr.c             |  28 ++++
>  drivers/soc/fsl/qbman/qman_portal.c           |  35 +++++
>  include/soc/fsl/bman.h                        |  16 +++
>  include/soc/fsl/qman.h                        |  17 +++
>  13 files changed, 438 insertions(+), 99 deletions(-)
> 
> --
> 2.17.1

For the fsl/fman and dpaa_eth:

Acked-by: Madalin Bucur <madalin.bucur@nxp.com>

Thank you

^ permalink raw reply

* Re: KASAN: use-after-free Read in tcf_block_find
From: Dmitry Vyukov @ 2018-09-27 13:42 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Cong Wang, syzbot+37b8770e6d5a8220a039, David Miller,
	Jamal Hadi Salim, Jiri Pirko, LKML,
	Linux Kernel Network Developers, syzkaller-bugs
In-Reply-To: <10a02f06-2e6c-e70b-2766-012a835d3a57@gmail.com>

On Thu, Sep 27, 2018 at 3:24 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On 09/27/2018 06:02 AM, Dmitry Vyukov wrote:
>
>> I am not suggesting to commit this. This is just a hack for debugging.
>> It in fact lead to some warnings, but still allowed me to reproduce
>> the bug reliably.
>>
>
> Had you got more meaningful stack traces ?
>
> (Showing which context was actually doing the dst_release())

nope... I just handed off the instructions to Wei since she said she
can't reproduce it.


>>>> +                               dst_destroy_rcu(&dst->rcu_head);
>>>> +                       }
>>>> +               }
>>>>         }
>>>>  }
>
> Thanks.

^ permalink raw reply

* [PATCH iproute2-next] geneve: fix ttl inherit behavior
From: Hangbin Liu @ 2018-09-27  7:27 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern, Phil Sutter, Hangbin Liu

Currently when we add geneve with "ttl inherit", we set ttl to 0, which
is actually use whatever default value instead of inherit the inner
protocol's ttl value.

To respect compatibility with old behavior and make a difference between
ttl inherit and ttl == 0, we add an attribute IFLA_GENEVE_TTL_INHERIT in
kernel commit 52d0d404d39dd ("geneve: add ttl inherit support").

Now let's use "ttl inherit" to inherit the inner protocol's ttl, and use
"ttl auto" to means "use whatever default value", the same behavior with
ttl == 0.

Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/if_link.h |  1 +
 ip/iplink_geneve.c           | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f4a9715..02d7bdf 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -552,6 +552,7 @@ enum {
 	IFLA_GENEVE_UDP_ZERO_CSUM6_TX,
 	IFLA_GENEVE_UDP_ZERO_CSUM6_RX,
 	IFLA_GENEVE_LABEL,
+	IFLA_GENEVE_TTL_INHERIT,
 	__IFLA_GENEVE_MAX
 };
 #define IFLA_GENEVE_MAX	(__IFLA_GENEVE_MAX - 1)
diff --git a/ip/iplink_geneve.c b/ip/iplink_geneve.c
index 26e70ff..c417842 100644
--- a/ip/iplink_geneve.c
+++ b/ip/iplink_geneve.c
@@ -34,7 +34,7 @@ static void print_explain(FILE *f)
 		"Where: VNI   := 0-16777215\n"
 		"       ADDR  := IP_ADDRESS\n"
 		"       TOS   := { NUMBER | inherit }\n"
-		"       TTL   := { 1..255 | inherit }\n"
+		"       TTL   := { 1..255 | auto | inherit }\n"
 		"       LABEL := 0-1048575\n"
 	);
 }
@@ -94,7 +94,9 @@ static int geneve_parse_opt(struct link_util *lu, int argc, char **argv,
 
 			NEXT_ARG();
 			check_duparg(&attrs, IFLA_GENEVE_TTL, "ttl", *argv);
-			if (strcmp(*argv, "inherit") != 0) {
+			if (strcmp(*argv, "inherit") == 0) {
+				addattr8(n, 1024, IFLA_GENEVE_TTL_INHERIT, 1);
+			} else if (strcmp(*argv, "auto") != 0) {
 				if (get_unsigned(&uval, *argv, 0))
 					invarg("invalid TTL", *argv);
 				if (uval > 255)
@@ -265,12 +267,16 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		}
 	}
 
-	if (tb[IFLA_GENEVE_TTL])
-		ttl = rta_getattr_u8(tb[IFLA_GENEVE_TTL]);
-	if (is_json_context() || ttl)
-		print_uint(PRINT_ANY, "ttl", "ttl %u ", ttl);
-	else
+	if (tb[IFLA_GENEVE_TTL_INHERIT] &&
+	    rta_getattr_u8(tb[IFLA_GENEVE_TTL_INHERIT])) {
 		print_string(PRINT_FP, NULL, "ttl %s ", "inherit");
+	} else if (tb[IFLA_GENEVE_TTL]) {
+		ttl = rta_getattr_u8(tb[IFLA_GENEVE_TTL]);
+		if (is_json_context() || ttl)
+			print_uint(PRINT_ANY, "ttl", "ttl %u ", ttl);
+		else
+			print_string(PRINT_FP, NULL, "ttl %s ", "auto");
+	}
 
 	if (tb[IFLA_GENEVE_TOS])
 		tos = rta_getattr_u8(tb[IFLA_GENEVE_TOS]);
-- 
2.5.5

^ permalink raw reply related

* [PATCH iproute2] vxlan: show correct ttl inherit info
From: Hangbin Liu @ 2018-09-27  7:28 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, David Ahern, Phil Sutter, Hangbin Liu

We should only show ttl inherit when IFLA_VXLAN_TTL_INHERIT supplied.
Otherwise show the ttl number, or auto when it is 0.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 ip/iplink_vxlan.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 831f39a..7fc0e2b 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -145,7 +145,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			check_duparg(&attrs, IFLA_VXLAN_TTL, "ttl", *argv);
 			if (strcmp(*argv, "inherit") == 0) {
-				addattr_l(n, 1024, IFLA_VXLAN_TTL_INHERIT, NULL, 0);
+				addattr(n, 1024, IFLA_VXLAN_TTL_INHERIT);
 			} else if (strcmp(*argv, "auto") == 0) {
 				addattr8(n, 1024, IFLA_VXLAN_TTL, ttl);
 			} else {
@@ -527,12 +527,16 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			print_string(PRINT_FP, NULL, "tos %s ", "inherit");
 	}
 
-	if (tb[IFLA_VXLAN_TTL])
-		ttl = rta_getattr_u8(tb[IFLA_VXLAN_TTL]);
-	if (is_json_context() || ttl)
-		print_uint(PRINT_ANY, "ttl", "ttl %u ", ttl);
-	else
+	if (tb[IFLA_VXLAN_TTL_INHERIT] &&
+	    rta_getattr_u8(tb[IFLA_VXLAN_TTL_INHERIT])) {
 		print_string(PRINT_FP, NULL, "ttl %s ", "inherit");
+	} else if (tb[IFLA_VXLAN_TTL]) {
+		ttl = rta_getattr_u8(tb[IFLA_VXLAN_TTL]);
+		if (is_json_context() || ttl)
+			print_uint(PRINT_ANY, "ttl", "ttl %u ", ttl);
+		else
+			print_string(PRINT_FP, NULL, "ttl %s ", "auto");
+	}
 
 	if (tb[IFLA_VXLAN_LABEL]) {
 		__u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
-- 
2.5.5

^ permalink raw reply related

* Re: [PATCH] team: set IFF_SLAVE on team ports
From: Chas Williams @ 2018-09-27 14:04 UTC (permalink / raw)
  To: Jiri Pirko, Jan Blunck; +Cc: LKML, netdev
In-Reply-To: <20150710064147.GA2204@nanopsycho.orion>



On 07/10/15 02:41, Jiri Pirko wrote:
> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>> decide if it should start the address configuration. Since team ports
>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>> a port to the team master.
>>>
>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>> not using that as well, for example bridge, ovs, etc.
>>>
>>
>> Maybe they need to get fixed too. I've used that flag because it is
>> documented as
>> a "slave of a load balancer" which describes what a team port is.
>>
>>
>>> I think that this should be fixed in addrconf_notify. It should lookup
>>> if there is a master on top and bail out in that case.
>>
>> There are other virtual interfaces that have a master assigned and want to
>> participate in IPv6 address configuration.
> 
> Can you give me an example?

I would like to revisit this patch (yes, I know it has been a while).  I 
believe the VRF implementation uses master to group the interfaces under
a single interface.

I don't see a reason not to use IFF_SLAVE since team and bonding are 
fairly similar.

>>
>> Unless we want to have a cascade of conditionals testing the priv_flags in
>> addrconf_notify() this is asking for a new net_device_flags flag.
>> Maybe something
>> generic like IFF_L2PORT ?
>>
>> Thanks,
>> Jan
>>
>> [ Jiri, sorry for getting that mail twice ]
> 
> 
> 

^ permalink raw reply

* Re: [PATCH v3 0/5] netlink: nested policy validation
From: Johannes Berg @ 2018-09-27  8:22 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, dsahern
In-Reply-To: <20180926.102154.1962354339786539715.davem@davemloft.net>

On Wed, 2018-09-26 at 10:21 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Wed, 26 Sep 2018 11:15:29 +0200
> 
> > This adds nested policy validation, which lets you specify the
> > nested attribute type, e.g. NLA_NESTED with sub-policy, or the
> > new NLA_NESTED_ARRAY with sub-sub-policy.
> > 
> > 
> > Changes in v2:
> >  * move setting the bad attr pointer/message into validate_nla()
> >  * remove the recursion patch since that's no longer needed
> >  * simply skip the generic bad attr pointer/message setting in
> >    case of nested nla_validate() failing since that could fail
> >    only due to validate_nla() failing inside, which already sets
> >    the extack information
> > 
> > Changes in v3:
> >  * fix NLA_REJECT to have an error message if none is in policy
> 
> Looks great Johannes, series applied.

Sorry to nag, but I see patches that you replied to later than this in
the tree, but not this.

Or did you see something wrong with this later and dropped it?

johannes

^ permalink raw reply

* Re: [PATCH] ieee802154: remove a redundant local variable 'i'
From: Stefan Schmidt @ 2018-09-27 14:47 UTC (permalink / raw)
  To: zhong jiang, davem; +Cc: alex.aring, netdev, linux-kernel
In-Reply-To: <1537368097-60101-1-git-send-email-zhongjiang@huawei.com>

Hello.

On 19/09/2018 16:41, zhong jiang wrote:
> The local variable 'i' is never used after being assigned.
> hence it should be redundant adn can be removed.
> 
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  net/ieee802154/nl802154.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index 99f6c25..5b90151 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -445,7 +445,6 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
>  {
>  	struct nlattr *nl_cmds;
>  	void *hdr;
> -	int i;
>  
>  	hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
>  	if (!hdr)
> @@ -508,7 +507,6 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
>  	if (!nl_cmds)
>  		goto nla_put_failure;
>  
> -	i = 0;
>  #define CMD(op, n)							\
>  	do {								\
>  		if (rdev->ops->op) {					\
> 

Sorry, but this patch is wrong. The variable i is used in line 513
inside the CMD() macro. The compiler clearly tells this when running
with your patch:

net/ieee802154/nl802154.c: In function ‘nl802154_send_wpan_phy’:
net/ieee802154/nl802154.c:513:4: error: ‘i’ undeclared (first use in
this function)

I would appreciate if patches sent out would at least be compile tested.

regards
Stefan Schmidt

^ permalink raw reply

* [PATCH] MAINTAINERS: change bridge maintainers
From: Stephen Hemminger @ 2018-09-27  8:47 UTC (permalink / raw)
  To: davem; +Cc: netdev, Stephen Hemminger, Stephen Hemminger

I haven't been doing reviews only but not active development on bridge
code for several years. Roopa and Nikolay have been doing most of
the new features and have agreed to take over as new co-maintainers.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7233a9ed0f5b..123ae2e65d94 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5470,7 +5470,8 @@ S:	Odd Fixes
 F:	drivers/net/ethernet/agere/
 
 ETHERNET BRIDGE
-M:	Stephen Hemminger <stephen@networkplumber.org>
+M:	Roopa Prabhu <roopa@cumulusnetworks.com>
+M:	Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
 L:	bridge@lists.linux-foundation.org (moderated for non-subscribers)
 L:	netdev@vger.kernel.org
 W:	http://www.linuxfoundation.org/en/Net:Bridge
-- 
2.17.1

^ permalink raw reply related

* Re: [PATCH net-next] virtio_net: ethtool tx napi configuration
From: Jason Wang @ 2018-09-27  8:50 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Florian Fainelli, Network Development, David Miller, caleb.raitto,
	Michael S. Tsirkin, Jon Olson (Google Drive), Willem de Bruijn
In-Reply-To: <CAF=yD-KEcWuYrSofugYBc3kwWvQdFDsv3z_yWx5-JDzyMga1qw@mail.gmail.com>



On 2018年09月14日 12:46, Willem de Bruijn wrote:
>> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
>> interrupt if we found the ring is about to full to avoid interrupt
>> storm, so we're probably ok in this case.
> I'm only concerned about the transition state when converting from
> napi to no-napi when the queue is stopped and tx interrupt disabled.
>
> With napi mode the interrupt is only disabled if napi is scheduled,
> in which case it will eventually reenable the interrupt. But when
> switching to no-napi mode in this state no progress will be made.
>
> But it seems this cannot happen. When converting to no-napi
> mode, set_coalesce waits for napi to complete in napi_disable.
> So the interrupt should always start enabled when transitioning
> into no-napi mode.

An update, I meet a hang in napi_disalbe(). But it's hard to be 
reproduced. I tend to choose a easy way like V1 that only allow the 
switching when device is down.

I will post the patch after a vacation. (or you can post if it was 
urgent for you).

Thanks

^ permalink raw reply

* Re: [PATCH] netlink: add policy attribute range validation
From: Johannes Berg @ 2018-09-27  8:51 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: linux-wireless, netdev
In-Reply-To: <20180927084807.GG30601@unicorn.suse.cz>

On Thu, 2018-09-27 at 10:48 +0200, Michal Kubecek wrote:

> We could still use helper macros so this part could become
> 
> DEFINE_NLA_U8_RANGE(retry_range, 1, 255);
> 
> or
> 
> DEFINE_NLA_RANGE(retry_range, u8, 1, 255);

True.

> > policy[] = {
> > 	...
> > 	[NL80211_ATTR_WIPHY_RETRY_SHORT] = {
> > 		.type = NLA_U8,
> > 		.validation_data = &retry_range,
> > 	},
> > 	...
> > };
> 
> And this could be also shortened using a macro.
> 
> It would still be longer but not that much.

Right. You'd still have to name it, and then we'd probably eventually
want to share some common range definitions, but indeed it would work.

> I didn't mean it as a serious objection, rather a note that the gain may
> not be worth the additional complexity. 

Sure, and suggestions are very welcome. I was just trying to explain why
I chose this direction, more than anything else.

> But if you want to follow in the
> direction you indicated later (in particular, allowing different
> interpretations of validation_data for the same type), overloading does
> indeed make more sense.

I'm just working on the patches - give me a few more minutes.

johannes

^ permalink raw reply

* Re: [PATCH] ieee802154: ca8210: remove redundant condition check before debugfs_remove
From: Stefan Schmidt @ 2018-09-27 15:15 UTC (permalink / raw)
  To: zhong jiang, h.morris, alex.aring, davem; +Cc: linux-wpan, netdev, linux-kernel
In-Reply-To: <1536901477-28375-1-git-send-email-zhongjiang@huawei.com>

Hello Zhong.

On 14/09/2018 07:04, zhong jiang wrote:
> debugfs_remove has taken the IS_ERR into account. Just
> remove the unnecessary condition.
> 
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  drivers/net/ieee802154/ca8210.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index 58299fb..46b4818 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -3044,8 +3044,7 @@ static void ca8210_test_interface_clear(struct ca8210_priv *priv)
>  {
>  	struct ca8210_test *test = &priv->test;
>  
> -	if (!IS_ERR(test->ca8210_dfs_spi_int))
> -		debugfs_remove(test->ca8210_dfs_spi_int);
> +	debugfs_remove(test->ca8210_dfs_spi_int);
>  	kfifo_free(&test->up_fifo);
>  	dev_info(&priv->spi->dev, "Test interface removed\n");
>  }
> 

This patch has been applied to the wpan tree and will be
part of the next pull request to net. Thanks!

regards
Stefan Schmidt

^ permalink raw reply

* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Jason A. Donenfeld @ 2018-09-27 15:19 UTC (permalink / raw)
  To: Thomas Gleixner, Peter Zijlstra
  Cc: Ard Biesheuvel, LKML, Netdev, Linux Crypto Mailing List,
	David Miller, Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
	Jean-Philippe Aumasson, Russell King - ARM Linux,
	linux-arm-kernel
In-Reply-To: <CAHmME9rN3-7Mj5JQqt2EFPauG9vjkN5pQn3tPiJY4fPiwksaDA@mail.gmail.com>

Hey again Thomas,

On Thu, Sep 27, 2018 at 3:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Thomas,
>
> I'm trying to optimize this for crypto performance while still taking
> into account preemption concerns. I'm having a bit of trouble figuring
> out a way to determine numerically what the upper bounds for this
> stuff looks like. I'm sure I could pick a pretty sane number that's
> arguably okay -- and way under the limit -- but I still am interested
> in determining what that limit actually is. I was hoping there'd be a
> debugging option called, "warn if preemption is disabled for too
> long", or something, but I couldn't find anything like that. I'm also
> not quite sure what the latency limits are, to just compute this with
> a formula. Essentially what I'm trying to determine is:
>
> preempt_disable();
> asm volatile(".fill N, 1, 0x90;");
> preempt_enable();
>
> What is the maximum value of N for which the above is okay? What
> technique would you generally use in measuring this?
>
> Thanks,
> Jason

>From talking to Peter (now CC'd) on IRC, it sounds like what you're
mostly interested in is clocktime latency on reasonable hardware, with
a goal of around ~20µs as a maximum upper bound? I don't expect to get
anywhere near this value at all, but if you can confirm that's a
decent ballpark, it would make for some interesting calculations.

Regards,
Jason

^ permalink raw reply

* Re: [PATCH resend] can: rcar_can: convert to SPDX identifiers
From: Marc Kleine-Budde @ 2018-09-27  9:05 UTC (permalink / raw)
  To: Kuninori Morimoto, Wolfgang Grandegger
  Cc: Linux-Renesas, Linux-Net, linux-can
In-Reply-To: <874ledyrc6.wl-kuninori.morimoto.gx@renesas.com>


[-- Attachment #1.1: Type: text/plain, Size: 819 bytes --]

On 09/26/2018 03:41 AM, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> This patch updates license to use SPDX-License-Identifier
> instead of verbose license text.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Wolfram Sang has already supplied a similar patch, but not for Makefile
and Kconfig. I've applied your patch for Makefile and Kconfig and
adjusted the commit message accordingly.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [PATCH] mt76: fix building without CONFIG_MT76x0U
From: kbuild test robot @ 2018-09-27 15:22 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: kbuild-all, Kalle Valo, Stanislaw Gruszka, Felix Fietkau,
	Lorenzo Bianconi, Arnd Bergmann, David S. Miller, linux-wireless,
	netdev, linux-kernel
In-Reply-To: <20180926125356.2026635-1-arnd@arndb.de>

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

Hi Arnd,

I love your patch! Yet something to improve:

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on next-20180926]
[cannot apply to v4.19-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Arnd-Bergmann/mt76-fix-building-without-CONFIG_MT76x0U/20180927-101346
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "mt76x0_tx_prepare_skb" [drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0u.ko] undefined!
>> ERROR: "mt76x0_cleanup" [drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0u.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65149 bytes --]

^ permalink raw reply

* Re: [PATCH iproute2-next] geneve: fix ttl inherit behavior
From: Phil Sutter @ 2018-09-27  9:08 UTC (permalink / raw)
  To: Hangbin Liu; +Cc: netdev, Stephen Hemminger, David Ahern
In-Reply-To: <1538033257-32244-1-git-send-email-liuhangbin@gmail.com>

On Thu, Sep 27, 2018 at 03:27:37PM +0800, Hangbin Liu wrote:
> Currently when we add geneve with "ttl inherit", we set ttl to 0, which
> is actually use whatever default value instead of inherit the inner
> protocol's ttl value.
> 
> To respect compatibility with old behavior and make a difference between
> ttl inherit and ttl == 0, we add an attribute IFLA_GENEVE_TTL_INHERIT in
> kernel commit 52d0d404d39dd ("geneve: add ttl inherit support").
> 
> Now let's use "ttl inherit" to inherit the inner protocol's ttl, and use
> "ttl auto" to means "use whatever default value", the same behavior with
> ttl == 0.
> 
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Acked-by: Phil Sutter <phil@nwl.cc>

^ permalink raw reply

* Re: [PATCH net V2] vhost-vsock: fix use after free
From: Stefan Hajnoczi @ 2018-09-27 15:33 UTC (permalink / raw)
  To: Jason Wang
  Cc: kvm, sergei.shtylyov, mst, netdev, linux-kernel, virtualization
In-Reply-To: <20180927122204.4188-1-jasowang@redhat.com>


[-- Attachment #1.1: Type: text/plain, Size: 838 bytes --]

On Thu, Sep 27, 2018 at 08:22:04PM +0800, Jason Wang wrote:
> The access of vsock is not protected by vhost_vsock_lock. This may
> lead to use after free since vhost_vsock_dev_release() may free the
> pointer at the same time.
> 
> Fix this by holding the lock during the access.
> 
> Reported-by: syzbot+e3e074963495f92a89ed@syzkaller.appspotmail.com
> Fixes: 16320f363ae1 ("vhost-vsock: add pkt cancel capability")
> Fixes: 433fc58e6bf2 ("VSOCK: Introduce vhost_vsock.ko")
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> - V2: fix typos
> - The patch is needed for -stable.
> ---
>  drivers/vhost/vsock.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Thank you, Jason!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH iproute2] vxlan: show correct ttl inherit info
From: Phil Sutter @ 2018-09-27  9:27 UTC (permalink / raw)
  To: Hangbin Liu; +Cc: netdev, Stephen Hemminger, David Ahern
In-Reply-To: <1538033316-32312-1-git-send-email-liuhangbin@gmail.com>

Hi Hangbin,

On Thu, Sep 27, 2018 at 03:28:36PM +0800, Hangbin Liu wrote:
> We should only show ttl inherit when IFLA_VXLAN_TTL_INHERIT supplied.
> Otherwise show the ttl number, or auto when it is 0.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  ip/iplink_vxlan.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> index 831f39a..7fc0e2b 100644
> --- a/ip/iplink_vxlan.c
> +++ b/ip/iplink_vxlan.c
> @@ -145,7 +145,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>  			NEXT_ARG();
>  			check_duparg(&attrs, IFLA_VXLAN_TTL, "ttl", *argv);
>  			if (strcmp(*argv, "inherit") == 0) {
> -				addattr_l(n, 1024, IFLA_VXLAN_TTL_INHERIT, NULL, 0);
> +				addattr(n, 1024, IFLA_VXLAN_TTL_INHERIT);

So for VXLAN, the attribute is just added but with a zero value. Looking
at respective kernel code, this seems fine. Now I wonder why for Geneve,
you set the value to 1 and when displaying explicitly check whether the
attribute is there *and* non-zero. OK, looks like Geneve driver always
exports IFLA_GENEVE_TTL_INHERIT. Oddly, I can't find where VXLAN driver
in kernel does export the IFLA_VXLAN_TTL_INHERIT attribute. Am I missing
something?
Do you know why handling of the attributes in both drivers differ?

Cheers, Phil

^ permalink raw reply

* [PATCH v2 0/2] netlink: extended attribute validation
From: Johannes Berg @ 2018-09-27  9:28 UTC (permalink / raw)
  To: linux-wireless, netdev; +Cc: Michal Kubecek

This adds further netlink attribute validation:
 * min/max/range validation
 * validation through a custom function pointer

This is useful to
 * reduce boilerplate code in command handling code, if attributes
   are used commonly across different commands
 * get more extended ACK error messages/attribute pointers
 * ensure attributes are valid even when ignored
   (though this might be a problem when converting existing code)

Changes since v1:
 * split off validate_type from type and use that for min/max/range
   and function; this is better because the range is limited to the
   range of s16 and so things like "u16 with minimum value 1" couldn't
   be expressed earlier
 * add macros for this, e.g. NLA_POLICY_MIN(NLA_U16, 1) for the case
   mentioned in the previous bullet

Using this pretty much in all places where applicable in nl80211
reduces the code size there by about 1.8KiB, with just a minimal
code increase in lib/nlattr.o.

johannes

^ permalink raw reply

* [PATCH v2 1/2] netlink: add attribute range validation to policy
From: Johannes Berg @ 2018-09-27  9:28 UTC (permalink / raw)
  To: linux-wireless, netdev; +Cc: Michal Kubecek, Johannes Berg
In-Reply-To: <20180927092836.6117-1-johannes@sipsolutions.net>

From: Johannes Berg <johannes.berg@intel.com>

Without further bloating the policy structs, we can overload
the `validation_data' pointer with a struct of s16 min, max
and use those to validate ranges in NLA_{U,S}{8,16,32,64}
attributes.

It may sound strange to validate NLA_U32 with a s16 max, but
in many cases NLA_U32 is used for enums etc. since there's no
size benefit in using a smaller attribute width anyway, due
to netlink attribute alignment; in cases like that it's still
useful, particularly when the attribute really transports an
enum value.

Doing so lets us remove quite a bit of validation code, if we
can be sure that these attributes aren't used by userspace in
places where they're ignored today.

To achieve all this, split the 'type' field and introduce a
new 'validation_type' field which indicates what further
validation (beyond the validation prescribed by the type of
the attribute) is done. This currently allows for no further
validation (the default), as well as min, max and range checks.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 67 ++++++++++++++++++++++++++++++++++++++++++++---
 lib/nlattr.c          | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 3 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 3698ca8ff92c..d34ceeba82a8 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -188,9 +188,19 @@ enum {
 
 #define NLA_TYPE_MAX (__NLA_TYPE_MAX - 1)
 
+enum nla_policy_validation {
+	NLA_VALIDATE_NONE,
+	NLA_VALIDATE_RANGE,
+	NLA_VALIDATE_MIN,
+	NLA_VALIDATE_MAX,
+};
+
 /**
  * struct nla_policy - attribute validation policy
  * @type: Type of attribute or NLA_UNSPEC
+ * @validation_type: type of attribute validation done in addition to
+ *	type-specific validation (e.g. range), see
+ *	&enum nla_policy_validation
  * @len: Type specific length of payload
  *
  * Policies are defined as arrays of this struct, the array must be
@@ -238,7 +248,26 @@ enum {
  *                         nested attributes directly inside, while an array has
  *                         the nested attributes at another level down and the
  *                         attributes directly in the nesting don't matter.
- *    All other            Unused
+ *    All other            Unused - but note that it's a union
+ *
+ * Meaning of `min' and `max' fields, use via NLA_POLICY_MIN, NLA_POLICY_MAX
+ * and NLA_POLICY_RANGE:
+ *    NLA_U8,
+ *    NLA_U16,
+ *    NLA_U32,
+ *    NLA_U64,
+ *    NLA_S8,
+ *    NLA_S16,
+ *    NLA_S32,
+ *    NLA_S64              These are used depending on the validation_type
+ *                         field, if that is min/max/range then the minimum,
+ *                         maximum and both are used (respectively) to check
+ *                         the value of the integer attribute.
+ *                         Note that in the interest of code simplicity and
+ *                         struct size both limits are s16, so you cannot
+ *                         enforce a range that doesn't fall within the range
+ *                         of s16 - do that as usual in the code instead.
+ *    All other            Unused - but note that it's a union
  *
  * Example:
  * static const struct nla_policy my_policy[ATTR_MAX+1] = {
@@ -249,9 +278,15 @@ enum {
  * };
  */
 struct nla_policy {
-	u16		type;
+	u8		type;
+	u8		validation_type;
 	u16		len;
-	const void     *validation_data;
+	union {
+		const void *validation_data;
+		struct {
+			s16 min, max;
+		};
+	};
 };
 
 #define NLA_POLICY_EXACT_LEN(_len)	{ .type = NLA_EXACT_LEN, .len = _len }
@@ -266,6 +301,32 @@ struct nla_policy {
 #define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \
 	{ .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr }
 
+#define __NLA_ENSURE(condition) (sizeof(char[1 - 2*!(condition)]) - 1)
+#define NLA_ENSURE_INT_TYPE(tp)				\
+	(__NLA_ENSURE(tp == NLA_S8 || tp == NLA_U8 ||	\
+		      tp == NLA_S16 || tp == NLA_U16 ||	\
+		      tp == NLA_S32 || tp == NLA_U32 ||	\
+		      tp == NLA_S64 || tp == NLA_U64) + tp)
+
+#define NLA_POLICY_RANGE(tp, _min, _max) {		\
+	.type = NLA_ENSURE_INT_TYPE(tp),		\
+	.validation_type = NLA_VALIDATE_RANGE,		\
+	.min = _min,					\
+	.max = _max					\
+}
+
+#define NLA_POLICY_MIN(tp, _min) {			\
+	.type = NLA_ENSURE_INT_TYPE(tp),		\
+	.validation_type = NLA_VALIDATE_MIN,		\
+	.min = _min,					\
+}
+
+#define NLA_POLICY_MAX(tp, _max) {			\
+	.type = NLA_ENSURE_INT_TYPE(tp),		\
+	.validation_type = NLA_VALIDATE_MAX,		\
+	.max = _max,					\
+}
+
 /**
  * struct nl_info - netlink source information
  * @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 2f8feff669a7..5670e4b7dfef 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -95,6 +95,64 @@ static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
 	return 0;
 }
 
+static int nla_validate_int_range(const struct nla_policy *pt,
+				  const struct nlattr *nla,
+				  struct netlink_ext_ack *extack)
+{
+	bool validate_min, validate_max;
+	s64 value;
+
+	validate_min = pt->validation_type == NLA_VALIDATE_RANGE ||
+		       pt->validation_type == NLA_VALIDATE_MIN;
+	validate_max = pt->validation_type == NLA_VALIDATE_RANGE ||
+		       pt->validation_type == NLA_VALIDATE_MAX;
+
+	switch (pt->type) {
+	case NLA_U8:
+		value = nla_get_u8(nla);
+		break;
+	case NLA_U16:
+		value = nla_get_u16(nla);
+		break;
+	case NLA_U32:
+		value = nla_get_u32(nla);
+		break;
+	case NLA_S8:
+		value = nla_get_s8(nla);
+		break;
+	case NLA_S16:
+		value = nla_get_s16(nla);
+		break;
+	case NLA_S32:
+		value = nla_get_s32(nla);
+		break;
+	case NLA_S64:
+		value = nla_get_s64(nla);
+		break;
+	case NLA_U64:
+		/* treat this one specially, since it may not fit into s64 */
+		if ((validate_min && nla_get_u64(nla) < pt->min) ||
+		    (validate_max && nla_get_u64(nla) > pt->max)) {
+			NL_SET_ERR_MSG_ATTR(extack, nla,
+					    "integer out of range");
+			return -ERANGE;
+		}
+		return 0;
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if ((validate_min && value < pt->min) ||
+	    (validate_max && value > pt->max)) {
+		NL_SET_ERR_MSG_ATTR(extack, nla,
+				    "integer out of range");
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy,
 			struct netlink_ext_ack *extack)
@@ -230,6 +288,20 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			goto out_err;
 	}
 
+	/* further validation */
+	switch (pt->validation_type) {
+	case NLA_VALIDATE_NONE:
+		/* nothing to do */
+		break;
+	case NLA_VALIDATE_RANGE:
+	case NLA_VALIDATE_MIN:
+	case NLA_VALIDATE_MAX:
+		err = nla_validate_int_range(pt, nla, extack);
+		if (err)
+			return err;
+		break;
+	}
+
 	return 0;
 out_err:
 	NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation");
-- 
2.14.4

^ permalink raw reply related

* [PATCH v2 2/2] netlink: add validation function to policy
From: Johannes Berg @ 2018-09-27  9:28 UTC (permalink / raw)
  To: linux-wireless, netdev; +Cc: Michal Kubecek, Johannes Berg
In-Reply-To: <20180927092836.6117-1-johannes@sipsolutions.net>

From: Johannes Berg <johannes.berg@intel.com>

Add the ability to have an arbitrary validation function attached
to a netlink policy that doesn't already use the validation_data
pointer in another way.

This can be useful to validate for example the content of a binary
attribute, like in nl80211 the "(information) elements", which must
be valid streams of "u8 type, u8 length, u8 value[length]".

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 24 +++++++++++++++++++++++-
 lib/nlattr.c          |  7 +++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index d34ceeba82a8..6a106ef5ca56 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -193,13 +193,14 @@ enum nla_policy_validation {
 	NLA_VALIDATE_RANGE,
 	NLA_VALIDATE_MIN,
 	NLA_VALIDATE_MAX,
+	NLA_VALIDATE_FUNCTION,
 };
 
 /**
  * struct nla_policy - attribute validation policy
  * @type: Type of attribute or NLA_UNSPEC
  * @validation_type: type of attribute validation done in addition to
- *	type-specific validation (e.g. range), see
+ *	type-specific validation (e.g. range, function call), see
  *	&enum nla_policy_validation
  * @len: Type specific length of payload
  *
@@ -269,6 +270,13 @@ enum nla_policy_validation {
  *                         of s16 - do that as usual in the code instead.
  *    All other            Unused - but note that it's a union
  *
+ * Meaning of `validate' field, use via NLA_POLICY_VALIDATE_FN:
+ *    NLA_BINARY           Validation function called for the attribute,
+ *                         not compatible with use of the validation_data
+ *                         as in NLA_BITFIELD32, NLA_REJECT, NLA_NESTED and
+ *                         NLA_NESTED_ARRAY.
+ *    All other            Unused - but note that it's a union
+ *
  * Example:
  * static const struct nla_policy my_policy[ATTR_MAX+1] = {
  * 	[ATTR_FOO] = { .type = NLA_U16 },
@@ -286,6 +294,8 @@ struct nla_policy {
 		struct {
 			s16 min, max;
 		};
+		int (*validate)(const struct nlattr *attr,
+				struct netlink_ext_ack *extack);
 	};
 };
 
@@ -307,6 +317,11 @@ struct nla_policy {
 		      tp == NLA_S16 || tp == NLA_U16 ||	\
 		      tp == NLA_S32 || tp == NLA_U32 ||	\
 		      tp == NLA_S64 || tp == NLA_U64) + tp)
+#define NLA_ENSURE_NO_VALIDATION_PTR(tp)		\
+	(__NLA_ENSURE(tp != NLA_BITFIELD32 &&		\
+		      tp != NLA_REJECT &&		\
+		      tp != NLA_NESTED &&		\
+		      tp != NLA_NESTED_ARRAY) + tp)
 
 #define NLA_POLICY_RANGE(tp, _min, _max) {		\
 	.type = NLA_ENSURE_INT_TYPE(tp),		\
@@ -327,6 +342,13 @@ struct nla_policy {
 	.max = _max,					\
 }
 
+#define NLA_POLICY_VALIDATE_FN(tp, fn, ...) {		\
+	.type = NLA_ENSURE_NO_VALIDATION_PTR(tp),	\
+	.validation_type = NLA_VALIDATE_FUNCTION,	\
+	.validate = fn,					\
+	.len = __VA_ARGS__ + 0,				\
+}
+
 /**
  * struct nl_info - netlink source information
  * @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 5670e4b7dfef..1e900bb414ef 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -300,6 +300,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		if (err)
 			return err;
 		break;
+	case NLA_VALIDATE_FUNCTION:
+		if (pt->validate) {
+			err = pt->validate(nla, extack);
+			if (err)
+				return err;
+		}
+		break;
 	}
 
 	return 0;
-- 
2.14.4

^ permalink raw reply related

* Re: kernel 4.18.5 Realtek 8111G network adapter stops responding under high system load
From: Ortwin Glück @ 2018-09-27 15:48 UTC (permalink / raw)
  To: Heiner Kallweit, David Arendt, Maciej S. Szmigiero, Gabriel C
  Cc: linux-kernel, nic_swsd, netdev
In-Reply-To: <968f03ee-a271-242b-d90a-5c70ea72ce3b@gmail.com>

On 25.09.18 23:03, Heiner Kallweit wrote:
> It seems that all chip versions from 34 (= RTL8168E-VL) with the
> exception of version 39 (= RTL8106E, first sub-version) need
> bit TXCFG_AUTO_FIFO.
> 
> And indeed, due to reordering of calls this bit is overwritten.
> Following patch moves setting the bit from the chip-specific
> hw_start function to rtl_set_tx_config_registers().
> 
> Whoever is hit by the issue and has the option to build a kernel,
> could you please test whether the patch fixes the issue for you?

Hi,

Looks good so far! No problems for almost 24 hours. This is on a router/firewall that links various 
sites via IPSec and other VPNs and has >10 network interfaces, 5 of which are Realtek ones.

Thanks,

Ortwin

# uname -a
Linux lofw 4.18.10+ #72 SMP PREEMPT Wed Sep 26 17:07:07 CEST 2018 x86_64 Intel(R) Core(TM) i5-7500 
CPU @ 3.40GHz GenuineIntel GNU/Linux
# uptime
  17:42:37 up 22:54,  1 user,  load average: 0.48, 0.38, 0.30
# ifconfig wan
wan: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
         inet 81.7.230.226  netmask 255.255.255.248  broadcast 81.7.230.231
         inet6 fe80::529a:4cff:fe2e:92be  prefixlen 64  scopeid 0x20<link>
         ether 50:9a:4c:2e:92:be  txqueuelen 100  (Ethernet)
         RX packets 56342905  bytes 40589502599 (37.8 GiB)
         RX errors 0  dropped 0  overruns 0  frame 0
         TX packets 54032328  bytes 44607761646 (41.5 GiB)
         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

# ifconfig lan
lan: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
         inet 10.11.1.1  netmask 255.255.255.0  broadcast 10.11.1.255
         inet6 fe80::20a:cdff:fe31:6022  prefixlen 64  scopeid 0x20<link>
         ether 00:0a:cd:31:60:22  txqueuelen 100  (Ethernet)
         RX packets 54799469  bytes 43111097607 (40.1 GiB)
         RX errors 0  dropped 0  overruns 0  frame 0
         TX packets 55158558  bytes 35746992802 (33.2 GiB)
         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox