* Re: [PATCH 0/4] docs: e100[0] fix build errors
From: Jeff Kirsher @ 2018-06-22 16:44 UTC (permalink / raw)
To: Tobin C. Harding, Jonathan Corbet
Cc: David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <20180622003708.31848-1-me@tobin.cc>
[-- Attachment #1: Type: text/plain, Size: 629 bytes --]
On Fri, 2018-06-22 at 10:37 +1000, Tobin C. Harding wrote:
> I may be a little confused here, I'm getting docs build failure on
> Linus' mainline, linux-next, and your docs-next but different errors.
> There seems to be patches to the first two trees that are not in your
> docs-next tree?
>
> Do networking docs typically go through your tree? Looks like
> networking has done some conversion to rst that hasn't gone through
> your
> tree. Or else my git skills are failing.
Networking documentation changes went through David Miller's networking
tree because he maintains changes under Documentation/networking/
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] Documentation: e100: Use correct heading adornment
From: Jeff Kirsher @ 2018-06-22 16:44 UTC (permalink / raw)
To: Tobin C. Harding, Jonathan Corbet
Cc: David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <20180622003708.31848-2-me@tobin.cc>
[-- Attachment #1: Type: text/plain, Size: 732 bytes --]
On Fri, 2018-06-22 at 10:37 +1000, Tobin C. Harding wrote:
> Recently documentation file was converted to rst. The document title
> has the incorrect heading adornment. From kernel docs:
>
> * Please stick to this order of heading adornments:
>
> 1. ``=`` with overline for document title::
>
> ==============
> Document title
> ==============
>
> Add overline heading adornment to document title.
>
> Fixes commit (85d63445f411 Documentation: e100: Update the Intel
> 10/100 driver doc)
>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] Documentation: e1000: Use correct heading adornment
From: Jeff Kirsher @ 2018-06-22 16:45 UTC (permalink / raw)
To: Tobin C. Harding, Jonathan Corbet
Cc: David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <20180622003708.31848-3-me@tobin.cc>
[-- Attachment #1: Type: text/plain, Size: 726 bytes --]
On Fri, 2018-06-22 at 10:37 +1000, Tobin C. Harding wrote:
> Recently documentation file was converted to rst. The document title
> has the incorrect heading adornment. From kernel docs:
>
> * Please stick to this order of heading adornments:
>
> 1. ``=`` with overline for document title::
>
> ==============
> Document title
> ==============
>
> Add overline heading adornment to document title.
>
> Fixes commit (228046e76189 Documentation: e1000: Update kernel
> documentation)
>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4] Documentation: e100: Fix docs build error
From: Jeff Kirsher @ 2018-06-22 16:46 UTC (permalink / raw)
To: Tobin C. Harding, Jonathan Corbet
Cc: David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <20180622003708.31848-4-me@tobin.cc>
[-- Attachment #1: Type: text/plain, Size: 970 bytes --]
On Fri, 2018-06-22 at 10:37 +1000, Tobin C. Harding wrote:
> Recent patch updated e100 docs to rst format. Docs build (`make
> htmldocs`) is currently failing due to this file with error:
>
> (SEVERE/4) Unexpected section title.
>
> This is because a section of the file is indented 2 spaces. Build
> error
> can be cleared by aligning the text with column 0. While we are
> changing
> these lines we can make sure line length does not exceed 72, that
> newlines following headings are uniform, and that full stops are
> followed by two spaces.
>
> Align text with column 0, limit line length to 72, ensure two spaces
> follow all full stops, ensure uniform use of newlines after heading.
>
> Fixes commit (85d63445f411 Documentation: e100: Update the Intel
> 10/100 driver doc)
>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] Documentation: e1000: Fix docs build error
From: Jeff Kirsher @ 2018-06-22 16:47 UTC (permalink / raw)
To: Tobin C. Harding, Jonathan Corbet
Cc: David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <20180622003708.31848-5-me@tobin.cc>
[-- Attachment #1: Type: text/plain, Size: 965 bytes --]
On Fri, 2018-06-22 at 10:37 +1000, Tobin C. Harding wrote:
> Recent patch updated e1000 docs to rst format. Docs build (`make
> htmldocs`) is currently failing due to this file with error:
>
> (SEVERE/4) Unexpected section title.
>
> This is because a section of the file is indented 2 spaces. Build
> error
> can be cleared by aligning the text with column 0. While we are
> changing
> these lines we can make sure line length does not exceed 72, that
> newlines following headings are uniform, and that full stops are
> followed by two spaces.
>
> Align text with column 0, limit line length to 72, ensure two spaces
> follow all full stops, ensure uniform use of newlines after heading.
>
> Fixes commit (228046e76189 Documentation: e1000: Update kernel
> documentation)
>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Fw: [Bug 200191] New: UBSAN: Undefined behaviour in ./include/net/xfrm.h:894
From: Stephen Hemminger @ 2018-06-22 16:54 UTC (permalink / raw)
To: steffen.klassert, herbert; +Cc: netdev
Begin forwarded message:
Date: Fri, 22 Jun 2018 15:20:06 +0000
From: bugzilla-daemon@bugzilla.kernel.org
To: stephen@networkplumber.org
Subject: [Bug 200191] New: UBSAN: Undefined behaviour in ./include/net/xfrm.h:894
https://bugzilla.kernel.org/show_bug.cgi?id=200191
Bug ID: 200191
Summary: UBSAN: Undefined behaviour in ./include/net/xfrm.h:894
Product: Networking
Version: 2.5
Kernel Version: v4.18-rc2
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Other
Assignee: stephen@networkplumber.org
Reporter: icytxw@gmail.com
Regression: No
static inline bool addr4_match(__be32 a1, __be32 a2, u8 prefixlen)
{
/* C99 6.5.7 (3): u32 << 32 is undefined behaviour */
if (sizeof(long) == 4 && prefixlen == 0)
return true;
return !((a1 ^ a2) & htonl(~0UL << (32 - prefixlen)));
}
$ cat report0
================================================================================
UBSAN: Undefined behaviour in ./include/net/xfrm.h:894:23
shift exponent -128 is negative
CPU: 0 PID: 6190 Comm: syz-executor1 Not tainted 4.18.0-rc1 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x122/0x1c8 lib/dump_stack.c:113
ubsan_epilogue+0x12/0x86 lib/ubsan.c:159
__ubsan_handle_shift_out_of_bounds+0x29a/0x2ff lib/ubsan.c:425
addr4_match include/net/xfrm.h:894 [inline]
__xfrm4_selector_match net/xfrm/xfrm_policy.c:77 [inline]
xfrm_selector_match+0xde9/0x11e0 net/xfrm/xfrm_policy.c:102
xfrm_sk_policy_lookup+0x179/0x460 net/xfrm/xfrm_policy.c:1178
xfrm_lookup+0x20e/0x1be0 net/xfrm/xfrm_policy.c:2149
xfrm_lookup_route+0x42/0x1f0 net/xfrm/xfrm_policy.c:2282
ip_route_output_flow+0x86/0xc0 net/ipv4/route.c:2588
udp_sendmsg+0x15c1/0x2180 net/ipv4/udp.c:1086
inet_sendmsg+0x103/0x490 net/ipv4/af_inet.c:798
sock_sendmsg_nosec net/socket.c:645 [inline]
sock_sendmsg+0xf9/0x180 net/socket.c:655
__sys_sendto+0x239/0x3c0 net/socket.c:1833
__do_sys_sendto net/socket.c:1845 [inline]
__se_sys_sendto net/socket.c:1841 [inline]
__x64_sys_sendto+0xef/0x1c0 net/socket.c:1841
do_syscall_64+0xb8/0x3a0 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x455a09
Code: 1d ba fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83
eb b9 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f0b710bdc68 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 00007f0b710be6d4 RCX: 0000000000455a09
RDX: 0000000000000000 RSI: 00000000200014c0 RDI: 0000000000000013
RBP: 000000000072bea0 R08: 0000000020001540 R09: 0000000000000010
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000000005d7 R14: 00000000006fdcc8 R15: 0000000000000000
================================================================================
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
EXT4-fs (sda): re-mounted. Opts: jqfmt=vfsold,
sr 1:0:0:0: [sr0] unaligned transfer
sr 1:0:0:0: [sr0] unaligned transfer
audit: type=1326 audit(1529680282.002:2): auid=4294967295 uid=0 gid=0
ses=4294967295 subj=kernel pid=6558 comm="syz-executor0" exe="/syz-executor0"
sig=31 arch=c000003e syscall=202 compat=0 ip=0x455a09 code=0x0
sr 1:0:0:0: [sr0] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sr 1:0:0:0: [sr0] tag#0 Sense Key : Not Ready [current]
sr 1:0:0:0: [sr0] tag#0 Add. Sense: Medium not present
sr 1:0:0:0: [sr0] tag#0 CDB: Read(10) 28 00 00 00 00 00 00 00 40 00
print_req_error: 25 callbacks suppressed
print_req_error: I/O error, dev sr0, sector 0
sr 1:0:0:0: [sr0] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sr 1:0:0:0: [sr0] tag#0 Sense Key : Not Ready [current]
sr 1:0:0:0: [sr0] tag#0 Add. Sense: Medium not present
sr 1:0:0:0: [sr0] tag#0 CDB: Read(10) 28 00 00 00 00 00 00 00 40 00
print_req_error: I/O error, dev sr0, sector 0
sr 1:0:0:0: [sr0] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sr 1:0:0:0: [sr0] tag#0 Sense Key : Not Ready [current]
sr 1:0:0:0: [sr0] tag#0 Add. Sense: Medium not present
sr 1:0:0:0: [sr0] tag#0 CDB: Read(10) 28 00 00 00 00 00 00 00 40 00
print_req_error: I/O error, dev sr0, sector 0
sr 1:0:0:0: [sr0] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sr 1:0:0:0: [sr0] tag#0 Sense Key : Not Ready [current]
sr 1:0:0:0: [sr0] tag#0 Add. Sense: Medium not present
sr 1:0:0:0: [sr0] tag#0 CDB: Read(10) 28 00 00 00 00 00 00 00 40 00
print_req_error: I/O error, dev sr0, sector 0
cgroup: cgroup2: unknown option ""
cgroup: cgroup2: unknown option ""
--
You are receiving this mail because:
You are the assignee for the bug.
^ permalink raw reply
* [PATCH] net: drivers/net: Convert random_ether_addr to eth_random_addr
From: Joe Perches @ 2018-06-22 17:51 UTC (permalink / raw)
To: Derek Chickles, Satanand Burla, Felix Manlunas, Raghu Vatsavayi,
Hans Ulli Kroll, Linus Walleij, Yisen Zhuang, Salil Mehta,
Jeff Kirsher, Bryan Whitehead, Microchip Linux Driver Support,
Harish Patil, Manish Chopra, Dept-GELinuxNICDev,
Solarflare linux maintainers, Edward Cree, Bert Kenward,
Grygorii Strashko, Wingman Kwok, Murali
Cc: b.a.t.m.a.n, netdev, linux-usb, linux-wireless, linux-kernel,
intel-wired-lan, Kalle Valo, linux-ntb, linux-omap,
linux-arm-kernel
random_ether_addr is a #define for eth_random_addr which is
generally preferred in kernel code by ~3:1
Convert the uses of random_ether_addr to enable removing the #define
Miscellanea:
o Convert &vfmac[0] to equivalent vfmac and avoid unnecessary line wrap
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/ethernet/cavium/liquidio/lio_main.c | 5 ++---
drivers/net/ethernet/cortina/gemini.c | 2 +-
drivers/net/ethernet/hisilicon/hip04_eth.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_common.c | 2 +-
drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c | 2 +-
drivers/net/ethernet/sfc/ef10_sriov.c | 2 +-
drivers/net/ethernet/ti/cpsw.c | 2 +-
drivers/net/ethernet/ti/netcp_core.c | 4 ++--
drivers/net/ntb_netdev.c | 2 +-
drivers/net/usb/lan78xx.c | 2 +-
drivers/net/wireless/ath/ath9k/hw.c | 2 +-
net/batman-adv/bridge_loop_avoidance.c | 2 +-
14 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 8a815bb57177..7cb4e753829b 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -3569,9 +3569,8 @@ static int setup_nic_devices(struct octeon_device *octeon_dev)
for (j = 0; j < octeon_dev->sriov_info.max_vfs; j++) {
u8 vfmac[ETH_ALEN];
- random_ether_addr(&vfmac[0]);
- if (__liquidio_set_vf_mac(netdev, j,
- &vfmac[0], false)) {
+ eth_random_addr(vfmac);
+ if (__liquidio_set_vf_mac(netdev, j, vfmac, false)) {
dev_err(&octeon_dev->pci_dev->dev,
"Error setting VF%d MAC address\n",
j);
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 6d7404f66f84..ce1f04fdbf70 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2435,7 +2435,7 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
port->mac_addr[0], port->mac_addr[1],
port->mac_addr[2]);
dev_info(dev, "using a random ethernet address\n");
- random_ether_addr(netdev->dev_addr);
+ eth_random_addr(netdev->dev_addr);
}
gmac_write_mac_address(netdev);
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 340e28211135..14374a856d30 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -904,7 +904,7 @@ static int hip04_mac_probe(struct platform_device *pdev)
hip04_config_port(ndev, SPEED_100, DUPLEX_FULL);
hip04_config_fifo(priv);
- random_ether_addr(ndev->dev_addr);
+ eth_random_addr(ndev->dev_addr);
hip04_update_mac_address(ndev);
ret = hip04_alloc_ring(ndev, d);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c944bd10b03d..95e9dfbe9839 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -11978,7 +11978,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
snprintf(netdev->name, IFNAMSIZ, "%.*sv%%d",
IFNAMSIZ - 4,
pf->vsi[pf->lan_vsi]->netdev->name);
- random_ether_addr(mac_addr);
+ eth_random_addr(mac_addr);
spin_lock_bh(&vsi->mac_filter_hash_lock);
i40e_add_mac_filter(vsi, mac_addr);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index dd947e4dd3ce..e1747a490066 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -828,7 +828,7 @@ static int lan743x_mac_init(struct lan743x_adapter *adapter)
}
if (!mac_address_valid)
- random_ether_addr(adapter->mac_address);
+ eth_random_addr(adapter->mac_address);
lan743x_mac_set_address(adapter, adapter->mac_address);
ether_addr_copy(netdev->dev_addr, adapter->mac_address);
return 0;
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_common.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_common.c
index 0c744b9c6e0a..77e386ebff09 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_common.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_common.c
@@ -212,7 +212,7 @@ int qlcnic_sriov_init(struct qlcnic_adapter *adapter, int num_vfs)
vp->max_tx_bw = MAX_BW;
vp->min_tx_bw = MIN_BW;
vp->spoofchk = false;
- random_ether_addr(vp->mac);
+ eth_random_addr(vp->mac);
dev_info(&adapter->pdev->dev,
"MAC Address %pM is configured for VF %d\n",
vp->mac, i);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index b9a7548ec6a0..0afc3d335d56 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -210,7 +210,7 @@ void rmnet_vnd_setup(struct net_device *rmnet_dev)
rmnet_dev->netdev_ops = &rmnet_vnd_ops;
rmnet_dev->mtu = RMNET_DFLT_PACKET_SIZE;
rmnet_dev->needed_headroom = RMNET_NEEDED_HEADROOM;
- random_ether_addr(rmnet_dev->dev_addr);
+ eth_random_addr(rmnet_dev->dev_addr);
rmnet_dev->tx_queue_len = RMNET_TX_QUEUE_LEN;
/* Raw IP mode */
diff --git a/drivers/net/ethernet/sfc/ef10_sriov.c b/drivers/net/ethernet/sfc/ef10_sriov.c
index 019cef1d3cf7..8820be83ce85 100644
--- a/drivers/net/ethernet/sfc/ef10_sriov.c
+++ b/drivers/net/ethernet/sfc/ef10_sriov.c
@@ -199,7 +199,7 @@ static int efx_ef10_sriov_alloc_vf_vswitching(struct efx_nic *efx)
return -ENOMEM;
for (i = 0; i < efx->vf_count; i++) {
- random_ether_addr(nic_data->vf[i].mac);
+ eth_random_addr(nic_data->vf[i].mac);
nic_data->vf[i].efx = NULL;
nic_data->vf[i].vlan = EFX_EF10_NO_VLAN;
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 358edab9e72e..093998124149 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2927,7 +2927,7 @@ static int cpsw_probe_dual_emac(struct cpsw_priv *priv)
dev_info(cpsw->dev, "cpsw: Detected MACID = %pM\n",
priv_sl2->mac_addr);
} else {
- random_ether_addr(priv_sl2->mac_addr);
+ eth_random_addr(priv_sl2->mac_addr);
dev_info(cpsw->dev, "cpsw: Random MACID = %pM\n",
priv_sl2->mac_addr);
}
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index e40aa3e31af2..6ebf110cd594 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -2052,7 +2052,7 @@ static int netcp_create_interface(struct netcp_device *netcp_device,
if (is_valid_ether_addr(efuse_mac_addr))
ether_addr_copy(ndev->dev_addr, efuse_mac_addr);
else
- random_ether_addr(ndev->dev_addr);
+ eth_random_addr(ndev->dev_addr);
devm_iounmap(dev, efuse);
devm_release_mem_region(dev, res.start, size);
@@ -2061,7 +2061,7 @@ static int netcp_create_interface(struct netcp_device *netcp_device,
if (mac_addr)
ether_addr_copy(ndev->dev_addr, mac_addr);
else
- random_ether_addr(ndev->dev_addr);
+ eth_random_addr(ndev->dev_addr);
}
ret = of_property_read_string(node_interface, "rx-channel",
diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 306a662eba94..df8d49ad48c3 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -430,7 +430,7 @@ static int ntb_netdev_probe(struct device *client_dev)
ndev->hw_features = ndev->features;
ndev->watchdog_timeo = msecs_to_jiffies(NTB_TX_TIMEOUT_MS);
- random_ether_addr(ndev->perm_addr);
+ eth_random_addr(ndev->perm_addr);
memcpy(ndev->dev_addr, ndev->perm_addr, ndev->addr_len);
ndev->netdev_ops = &ntb_netdev_ops;
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 8dff87ec6d99..a89570f34937 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1720,7 +1720,7 @@ static void lan78xx_init_mac_address(struct lan78xx_net *dev)
"MAC address read from EEPROM");
} else {
/* generate random MAC */
- random_ether_addr(addr);
+ eth_random_addr(addr);
netif_dbg(dev, ifup, dev->net,
"MAC address set to random addr");
}
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index e60bea4604e4..1665066f4e24 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -496,7 +496,7 @@ static void ath9k_hw_init_macaddr(struct ath_hw *ah)
ath_err(common, "eeprom contains invalid mac address: %pM\n",
common->macaddr);
- random_ether_addr(common->macaddr);
+ eth_random_addr(common->macaddr);
ath_err(common, "random mac address will be used: %pM\n",
common->macaddr);
diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index a2de5a44bd41..ff9659af6b91 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -1449,7 +1449,7 @@ static void batadv_bla_periodic_work(struct work_struct *work)
* detection frames. Set the locally administered bit to avoid
* collisions with users mac addresses.
*/
- random_ether_addr(bat_priv->bla.loopdetect_addr);
+ eth_random_addr(bat_priv->bla.loopdetect_addr);
bat_priv->bla.loopdetect_addr[0] = 0xba;
bat_priv->bla.loopdetect_addr[1] = 0xbe;
bat_priv->bla.loopdetect_lasttime = jiffies;
--
2.15.0
^ permalink raw reply related
* Re: bnx2x: kernel panic in the bnx2x driver
From: Vishwanath Pai @ 2018-06-22 18:27 UTC (permalink / raw)
To: Sudarsana.Kalluru, Ariel.Elior, Dept-EngEverestLinuxL2
Cc: davem, netdev, dbanerje, pai.vishwain
The patch below worked for me (on 4.14.51 LTS kernel):
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 1e33abd..2b3863a 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -3387,14 +3387,20 @@ static int bnx2x_set_rss_flags(struct bnx2x *bp, struct ethtool_rxnfc *info)
DP(BNX2X_MSG_ETHTOOL,
"rss re-configured, UDP 4-tupple %s\n",
udp_rss_requested ? "enabled" : "disabled");
- return bnx2x_rss(bp, &bp->rss_conf_obj, false, true);
+ if (bp->state == BNX2X_STATE_OPEN)
+ return bnx2x_rss(bp, &bp->rss_conf_obj, false, true);
+ else
+ return 0;
} else if ((info->flow_type == UDP_V6_FLOW) &&
(bp->rss_conf_obj.udp_rss_v6 != udp_rss_requested)) {
bp->rss_conf_obj.udp_rss_v6 = udp_rss_requested;
DP(BNX2X_MSG_ETHTOOL,
"rss re-configured, UDP 4-tupple %s\n",
udp_rss_requested ? "enabled" : "disabled");
- return bnx2x_rss(bp, &bp->rss_conf_obj, false, true);
+ if (bp->state == BNX2X_STATE_OPEN)
+ return bnx2x_rss(bp, &bp->rss_conf_obj, false, true);
+ else
+ return 0;
}
return 0;
Although I think there might be another place where we may need to fix this as well:
bnx2x_config_rss_eth()
Thanks,
Vishwanath
On 06/22/2018 10:57 AM, Vishwanath Pai wrote:
> Ah, that is great! I will test it out on my machine and let you know.
>
> Thanks,
> Vishwanath
>
> On 06/22/2018 10:21 AM, Kalluru, Sudarsana wrote:
>> Hi Vishwanath,
>> The config will be cached in the device structure (bp->rss_conf_obj.udp_rss_v4) in this scenario, and will be applied in the load path (bnx2x_nic_load() --> bnx2x_init_rss()). Have unit tested the change on my setup.
>>
>> Thanks,
>> Sudarsana
>>
>> -----Original Message-----
>> From: Vishwanath Pai [mailto:vpai@akamai.com]
>> Sent: 22 June 2018 18:52
>> To: Kalluru, Sudarsana <Sudarsana.Kalluru@cavium.com>; Elior, Ariel <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2 <Dept-EngEverestLinuxL2@cavium.com>
>> Cc: davem@davemloft.net; netdev@vger.kernel.org; dbanerje@akamai.com; pai.vishwain@gmail.com
>> Subject: Re: bnx2x: kernel panic in the bnx2x driver
>>
>> Hi Sudarsana,
>>
>> Thanks for taking a look at my email. The fix you suggested would definitely fix the kernel panic, but at the same time wouldn't it also silently ignore the request by ethtool to set rx-flow-hash?
>>
>> Thanks,
>> Vishwanath
>>
>> On 06/22/2018 06:20 AM, Kalluru, Sudarsana wrote:
>>> Hi Vishwanath,
>>> Thanks for your mail, and the analysis.
>>> The fix would be to invoke bnx2x_rss() only when the device is opened,
>>> if (bp->state == BNX2X_STATE_OPEN)
>>> return bnx2x_rss(bp, &bp->rss_conf_obj, false, true);
>>> else
>>> return 0;
>>> Ariel,
>>> Could you please review the path (bnx2x_set_rss_flags()--> bnx2x_rss()) and confirm/correct on the above.
>>>
>>> Thanks,
>>> Sudarsana
>>>
>>> -----Original Message-----
>>> From: Vishwanath Pai [mailto:vpai@akamai.com]
>>> Sent: 22 June 2018 10:37
>>> To: Elior, Ariel <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2
>>> <Dept-EngEverestLinuxL2@cavium.com>
>>> Cc: davem@davemloft.net; netdev@vger.kernel.org; dbanerje@akamai.com;
>>> pai.vishwain@gmail.com
>>> Subject: bnx2x: kernel panic in the bnx2x driver
>>>
>>> External Email
>>>
>>> Hi,
>>>
>>> We recently noticed a kernel panic in the bnx2x driver when trying to
>>> set rx-flow-hash parameters via ethtool during if-pre-up.d. I am
>>> running kernel
>>> v4.17.2 from ubuntu-mainline-ppa. I have added the stack trace below:
>>>
>>> [ 18.280209] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
>>> [ 18.280212] PGD 8000000407a79067 P4D 8000000407a79067 PUD 40ce8a067 PMD 0
>>> [ 18.280214] Oops: 0010 [#1] SMP PTI
>>> [ 18.280215] Modules linked in: intel_rapl x86_pkg_temp_thermal intel_powerclamp kvm_intel joydev input_led kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc hid_eneric aesni_intel gpio_ich aes_x86_64 usbhid lpc_ich crpto_simd ie31200_edac cryptd glue_helper intel_cstate mac_hid intel_rapl_perf bnx2x mdio tcp_bbr netconsole ipmi_devintf ipmi_msghandler i2c_i801 coretemp autofs4 raid10 raid456 libcrc32c async_raid6_recov async_memcpy async_pq async_xor xor async_tx raid6_pq raid1 raid0 multipath linear sha26_mb mcryptd sha256_ssse3 hid ast i2c_algo_bit ttm drm_kms_helper syscopyarea sysfillrect sysimgblt mpt3sas fb_sys_fops drm raid_class scsi_transport_sas ahci libahci shpchp video
>>> [ 18.280241] CPU: 6 PID: 1081 Comm: ethtool Not tainted 4.17.2-041702-generic #201806160433
>>> [ 18.280242] Hardware name: Foxconn CangJie/CangJie, BIOS CC1F108D 02/26/2014
>>> [ 18.280243] RIP: 0010: (null)
>>> [ 18.280243] RSP: 0018:ffffb84bc260b9c0 EFLAGS: 00010246
>>> [ 18.280244] RAX: 0000000000000000 RBX: ffff92f987f020f0 RCX: 0000000000000000
>>> [ 18.280245] RDX: 0000000000000000 RSI: ffffb84bc260b9f8 RDI: ffff92f987f020f0
>>> [ 18.280245] RBP: ffffb8bc260b9e8 R08: 0000000000000001 R09: 0000000000000000
>>> [ 18.280246] R10: ffffb84bc260bd20 R11: 0000000000000000 R12: ffffb84bc260b9f8
>>> [ 18.280246] R13: ffff92f987f008c0 R14: 00007ffdb75bec40 R15: 0000000000000000
>>> [ 18.280247] FS: 00007fc0e8798700(0000) GS:ffff92f99fd80000(0000) knlGS:0000000000000000
>>> [ 18.280248] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [ 18.280249] CR2: 0000000000000000 CR3: 0000000409b4c003 CR4: 00000000001606e0
>>> [ 18.280249] Call Trace:
>>> [ 18.280263] ? bnx2x_config_rss+0x2f/0xd0 [bnx2x]
>>> [ 18.280270] bnx2x_rss+0x1d9/0x210 [bnx2x]
>>> [ 18.280276] bnx2x_set_rxnfc+0x17d/0x380 [bnx2x]
>>> [ 18.280279] ethtool_set_rxnfc+0x9b/0x110
>>> [ 18.280281] ? __do_page_cache_readahead+0x1da/0x2c0
>>> [ 18.280283] ? security_capable+0x3c/0x60
>>> [ 18.280284] dev_ethtool+0350/0x2610
>>> [ 18.280286] ? page_cache_async_readahead+0x71/0x80
>>> [ 18.280288] ? page_add_file_rmap+0x5d/0x220
>>> [ 18.280290] ? inet_ioctl+0x182/0x1a0
>>> [ 18.280291] dev_ioctl+0x203/0x3f0
>>> [ 18.280293] ? dev_ioctl+0x203/0x3f0
>>> [ 18.280294] sock_do_ioctl+0xae/0x150
>>> [ 18.280296] sock_ioctl+0x1e2/0x330
>>> [ 18.280296] ? sock_ioctl+0x1e2/0x330
>>> [ 18.280299] do_vfs_ioctl+0xa8/0x620
>>> [ 18.280300] ? dlci_ioctl_set+0x30/0x30
>>> [ 18.280301] ? do_vfs_ioctl+0xa8/0x620
>>> [ 18.280302] ? handle_mm_fault+0xe3/0x220
>>> [ 18.280304] ksys_ioctl+0x75/0x80
>>> [ 18.280305] __x64_sys_ioctl+0x1a/0x20
>>> [ 18.280307] do_syscall_64+0x5a/0x120
>>> [ 18.280309] entry_SYSCALL_64_aftr_hwframe+0x44/0xa9
>>> [ 18.280310] RIP: 0033:0x7fc0e7fba107
>>> [ 18.280311] RSP: 002b:00007ffdb75beb78 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
>>> [ 18.280312] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fc0e7fba107
>>> [ 18.280312] RDX: 00007ffdb75bed60 RSI: 0000000000008946 RDI: 0000000000000003
>>> [ 18.280313] RBP: 00007ffdb75bed50 R08: 00007ffdb75bed60 R09: 0000000000000001
>>> [ 18.280313] R10: 0000000000000541 R11: 0000000000000206 R12: 00007ffdb75beed0
>>> [ 18.280314] R13: 0000000000421020 R14: 000000000041fe28 R15: 0000000000000003
>>> [ 18.280315] Code: Bad RIP value.
>>> [ 18.280317] RIP: (null) RSP: ffffb84bc260b9c0
>>> [ 18.280318] CR2: 0000000000000000
>>> [ 18.280319] ---[ end trace 5f361db3fb9059f1 ]---
>>>
>>> To reproduce this I created a bash script in "/etc/network/if-pre-up.d/" with these two lines:
>>> ethtool -N $IFACE rx-flow-hash udp4 "sdfn"
>>> ethtool -N $IFACE rx-flow-hash udp6 "sdfn"
>>>
>>> The problem here is that rss_obj in bnx2x struct for the device hasn't
>>> been initialized yet, which causes an exception in bnx2x_config_rss()
>>> when calling "r->set_pending(r)" because r->set_pending is NULL. It
>>> looks like a lot many things haven't been initialized at this point,
>>> most of that happens in this
>>> function: "bnx2x_init_bp_objs()" which isn't called until ifup. Any thoughts on how this can be fixed? Would it be possible to safely move bnx2x_init_bp_objs() to maybe bnx2x_init_one() which runs much before ifup?
>>>
>>> Thanks,
>>> Vishwanath
>>>
>
^ permalink raw reply related
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Jakub Kicinski @ 2018-06-22 18:40 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov, Yonghong Song,
Quentin Monnet, David S. Miller, netdev, kernel-team,
linux-kernel
In-Reply-To: <20180622012052.htkvholi674x6i4f@kafai-mbp.dhcp.thefacebook.com>
On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > $ sudo bpftool map dump -p id 14
> > > > > > > [{
> > > > > > > "key": 0
> > > > > > > },{
> > > > > > > "value": {
> > > > > > > "m": 1,
> > > > > > > "n": 2,
> > > > > > > "o": "c",
> > > > > > > "p": [15,16,17,18,15,16,17,18
> > > > > > > ],
> > > > > > > "q": [[25,26,27,28,25,26,27,28
> > > > > > > ],[35,36,37,38,35,36,37,38
> > > > > > > ],[45,46,47,48,45,46,47,48
> > > > > > > ],[55,56,57,58,55,56,57,58
> > > > > > > ]
> > > > > > > ],
> > > > > > > "r": 1,
> > > > > > > "s": 0x7ffff6f70568,
> > > > > > > "t": {
> > > > > > > "x": 5,
> > > > > > > "y": 10
> > > > > > > },
> > > > > > > "u": 100,
> > > > > > > "v": 20,
> > > > > > > "w1": 0x7,
> > > > > > > "w2": 0x3
> > > > > > > }
> > > > > > > }
> > > > > > > ]
> > > > > >
> > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > JSON must remain backwards compatible.
> > > > > >
> > > > > > The dump today has object per entry, e.g.:
> > > > > >
> > > > > > {
> > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > ],
> > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > ]
> > > > > > }
> > > > > >
> > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > >
> > > > > > {
> > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > ],
> > > > > > "key_struct":{
> > > > > > "index":0
> > > > > > },
> Got a few questions.
>
> When we support hashtab later, the key could be int
> but reusing the name as "index" is weird.
Ugh, yes, naturally. I just typed that out without thinking, so for
array maps there is usually no BTF info?... For hashes obviously we
should just use the BTF, I'm not sure we should format indexes for
arrays nicely or not :S
> The key could also be a struct (e.g. a struct to describe ip:port).
> Can you suggest how the "key_struct" will look like?
Hm. I think in my mind it has only been a struct but that's not true :S
So the struct in the name makes very limited sense now.
Should we do:
"formatted" : {
"value" : XXX
}
Where
XXX == plain int for integers, e.g. "value":0
XXX == array for arrays, e.g. "value":[1,2,3,4]
XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
> > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > ],
> > > > > > "value_struct":{
> > > > > > "src_ip":2,
> If for the same map the user changes the "src_ip" to an array of int[4]
> later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> Is it breaking backward compat?
> i.e.
> struct five_tuples {
> - int src_ip;
> + int src_ip[4];
> /* ... */
> };
Well, it is breaking backward compat, but it's the program doing it,
not bpftool :) BTF changes so does the output.
> > > > > > "dst_ip:0
> > > > > > }
> > > > > > }
> > > > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > > > while most people would prefer "key_struct"/"value_struct" if it is
> > > > > available.
> > > >
> > > > Agreed, it's not that useful, especially with the string-hex debacle :(
> > > > It's just about the backwards compat.
> > > >
> > > > > How about introducing a new option, like "-b", to print the
> > > > > map with BTF (if available) such that it won't break the existing
> > > > > one (-j or -p) while the "-b" output can keep using the "key"
> > > > > and "value".
> > > > >
> > > > > The existing json can be kept as is.
> > > >
> > > > That was my knee jerk reaction too, but on reflection it doesn't sound
> > > > that great. We expect people with new-enough bpftool to use btf, so it
> > > > should be available in the default output, without hiding it behind a
> > > > switch. We could add a switch to hide the old output, but that doesn't
> > > > give us back the names... What about Key and Value or k and v? Or
> > > > key_fields and value_fields?
> > > I thought the current default output is "plain" ;)
> > > Having said that, yes, the btf is currently printed in json.
> > >
> > > Ideally, the default json output should do what most people want:
> > > print btf and btf only (if it is available).
> > > but I don't see a way out without new option if we need to
> > > be backward compat :(
> > >
> > > Agree that showing the btf in the existing json output will be useful (e.g.
> > > to hint people that BTF is available). If btf is showing in old json,
> > > also agree that the names should be the same with the new json.
> > > key_fields and value_fields may hint it has >1 fields though.
> > > May be "formatted_key" and "formatted_value"?
> >
> > SGTM! Or even maybe as a "formatted" object?:
> >
> > {
> > "key":["0x00","0x00","0x00","0x00",
> > ],
> > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > ],
> > "formatted":{
> > "key":{
> > "index":0
> > },
> > "value":{
> > "src_ip":2,
> > "dst_ip:0
> > }
> > }
> hmm... that is an extra indentation (keep in mind that the "value" could
> already have a few nested structs which itself consumes a few indentations)
> but I guess adding another one may be ok-ish.
I'm not fussed about this, whatever JSON writer does by default is fine
with me, really.
> > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > better one?
> > > > > >
> > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > >
> > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > bpftool patches before posting.
> > > > > >
> > > > > > Thanks for working on this!
> >
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Jakub Kicinski @ 2018-06-22 18:43 UTC (permalink / raw)
To: Okash Khawaja
Cc: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
In-Reply-To: <20180622111751.GB3050@w1t1fb>
On Fri, 22 Jun 2018 12:17:52 +0100, Okash Khawaja wrote:
> > > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > > better one?
> > > > > > >
> > > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > > >
> > > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > > bpftool patches before posting.
> > > > > > >
> > > > > > > Thanks for working on this!
> > >
>
> Hi,
>
> While I agree on the point of backward compatibility, I think printing
> two overlapping pieces of information side-by-side will make the
> interface less clear. Having separate outputs for the two will keep the
> interface clear and readable.
>
> Is there a major downside to adding a new flag for BTF output?
plain output and JSON should be more or less equivalent, just formatted
differently. If we hide the BTF-ed JSON under some flag most users
will just run bpftool -jp map dump id X, see BTF is not there and move
on, without ever looking into the man page to discover there is a magic
switch...
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Jakub Kicinski @ 2018-06-22 18:44 UTC (permalink / raw)
To: Quentin Monnet
Cc: Okash Khawaja, Daniel Borkmann, Martin KaFai Lau,
Alexei Starovoitov, Yonghong Song, David S. Miller, netdev,
kernel-team, linux-kernel
In-Reply-To: <a598aea7-e967-1779-5092-937565a69c0b@netronome.com>
On Fri, 22 Jun 2018 11:39:13 +0100, Quentin Monnet wrote:
> 2018-06-22 11:24 UTC+0100 ~ Okash Khawaja <osk@fb.com>
> > On Thu, Jun 21, 2018 at 11:42:59AM +0100, Quentin Monnet wrote:
> >> Hi Okash,
> > hi and sorry about delay in responding. the email got routed to
> > incorrect folder.
> >>
> >> 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@fb.com>
> [...]
> >>
> >> Hexadecimal values, without quotes, are not valid JSON. Please stick to
> >> decimal values.
> > ah sorry, i used a buggy json validator. this should be a quick fix.
> > which would be better: pointers be output hex strings or integers?
>
> I would go for integers. Although this is harder to read for humans, it
> is easier to process for machines, which remain the primary targets for
> JSON output.
+1
^ permalink raw reply
* [PATCH] selftests: bpf: notification about privilege required to run test_lirc_mode2.sh testing script
From: Jeffrin Jose T @ 2018-06-22 18:54 UTC (permalink / raw)
To: ast, daniel, shuah; +Cc: netdev, linux-kernel, linux-kselftest, Jeffrin Jose T
The test_lirc_mode2.sh script require root privilege for the successful
execution of the test.
This patch is to notify the user about the privilege the script
demands for the successful execution of the test.
Signed-off-by: Jeffrin Jose T (Rajagiri SET) <ahiliation@gmail.com>
---
tools/testing/selftests/bpf/test_lirc_mode2.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_lirc_mode2.sh b/tools/testing/selftests/bpf/test_lirc_mode2.sh
index ce2e15e4f976..51184f8f9e64 100755
--- a/tools/testing/selftests/bpf/test_lirc_mode2.sh
+++ b/tools/testing/selftests/bpf/test_lirc_mode2.sh
@@ -1,6 +1,15 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+msg="skip all tests:"
+if [ $UID != 0 ]; then
+ echo $msg please run this as root >&2
+ exit $ksft_skip
+fi
+
GREEN='\033[0;92m'
RED='\033[0;31m'
NC='\033[0m' # No Color
--
2.17.0
^ permalink raw reply related
* [PATCH bpf] nfp: bpf: don't stop offload if replace failed
From: Jakub Kicinski @ 2018-06-22 18:56 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, oss-drivers, Jakub Kicinski
Stopping offload completely if replace of program failed dates
back to days of transparent offload. Back then we wanted to
silently fall back to the in-driver processing. Today we mark
programs for offload when they are loaded into the kernel, so
the transparent offload is no longer a reality.
Flags check in the driver will only allow replace of a driver
program with another driver program or an offload program with
another offload program.
When driver program is replaced stopping offload is a no-op,
because driver program isn't offloaded. When replacing
offloaded program if the offload fails the entire operation
will fail all the way back to user space and we should continue
using the old program. IOW when replacing a driver program
stopping offload is unnecessary and when replacing offloaded
program - it's a bug, old program should continue to run.
In practice this bug would mean that if offload operation was to
fail (either due to FW communication error, kernel OOM or new
program being offloaded but for a different netdev) driver
would continue reporting that previous XDP program is offloaded
but in fact no program will be loaded in hardware. The failure
is fairly unlikely (found by inspection, when working on the code)
but it's unpleasant.
Backport note: even though the bug was introduced in commit
cafa92ac2553 ("nfp: bpf: add support for XDP_FLAGS_HW_MODE"),
this fix depends on commit 441a33031fe5 ("net: xdp: don't allow
device-bound programs in driver mode"), so this fix is sufficient
only in v4.15 or newer. Kernels v4.13.x and v4.14.x do need to
stop offload if it was transparent/opportunistic, i.e. if
XDP_FLAGS_HW_MODE was not set on running program.
Fixes: cafa92ac2553 ("nfp: bpf: add support for XDP_FLAGS_HW_MODE")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index fcdfb8e7fdea..7184af94aa53 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -81,10 +81,10 @@ nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
ret = nfp_net_bpf_offload(nn, prog, running, extack);
/* Stop offload if replace not possible */
- if (ret && prog)
- nfp_bpf_xdp_offload(app, nn, NULL, extack);
+ if (ret)
+ return ret;
- nn->dp.bpf_offload_xdp = prog && !ret;
+ nn->dp.bpf_offload_xdp = !!prog;
return ret;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] net: bridge: fix potential null pointer dereference on return from br_port_get_rtnl()
From: Garry McNulty @ 2018-06-22 19:05 UTC (permalink / raw)
To: nikolay
Cc: davem, netdev, stephen, Jiří Pírko, bridge,
linux-kernel
In-Reply-To: <e4fb2883-1643-a921-a138-d1255831a9bd@cumulusnetworks.com>
On Fri, 22 Jun 2018 at 00:35, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
>
> On 06/22/2018 01:20 AM, David Miller wrote:
> > From: Garry McNulty <garrmcnu@gmail.com>
> > Date: Thu, 21 Jun 2018 21:14:27 +0100
> >
> >> br_port_get_rtnl() can return NULL if the network device is not a bridge
> >> port (IFF_BRIDGE_PORT flag not set). br_port_slave_changelink() and
> >> br_port_fill_slave_info() callbacks dereference this pointer without
> >> checking. Currently this is not a problem because slave devices always
> >> set this flag. Add null check in case these conditions ever changye.
> >>
> >> Detected by CoverityScan, CID 1339613 ("Dereference null return value")
> >>
> >> Signed-off-by: Garry McNulty <garrmcnu@gmail.com>
> >
> > I don't think this is reasonable.
> >
> > The bridge code will never, ever, install a slave that doesn't have
> > that bit set. It's the most fundamental aspect of how these objects
> > are managed.
> >
> +1
>
> This keeps coming up, here's the previous one:
> https://patchwork.ozlabs.org/patch/896046/
>
> Please do a more thorough check if these conditions can actually occur.
> In this case, as Dave said, they cannot.
>
> To be explicit as with the patch I mentioned above:
> Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
> You can find more info in my reply to the patch above.
>
> Thanks,
> Nik
Thanks for reviewing and for the feedback.
Regards
Garry
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-22 19:05 UTC (permalink / raw)
To: Cornelia Huck
Cc: Siwei Liu, Samudrala, Sridhar, Alexander Duyck, virtio-dev,
aaron.f.brown, Jiri Pirko, Jakub Kicinski, Netdev, qemu-devel,
virtualization, konrad.wilk, boris.ostrovsky, Joao Martins,
Venu Busireddy, vijay.balakrishna
In-Reply-To: <20180622170955.298900c1.cohuck@redhat.com>
On Fri, Jun 22, 2018 at 05:09:55PM +0200, Cornelia Huck wrote:
> On Thu, 21 Jun 2018 21:20:13 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Thu, Jun 21, 2018 at 04:59:13PM +0200, Cornelia Huck wrote:
> > > OK, so what about the following:
> > >
> > > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
> > > that we have a new uuid field in the virtio-net config space
> > > - in QEMU, add a property for virtio-net that allows to specify a uuid,
> > > offer VIRTIO_NET_F_STANDBY_UUID if set
> > > - when configuring, set the property to the group UUID of the vfio-pci
> > > device
> > > - in the guest, use the uuid from the virtio-net device's config space
> > > if applicable; else, fall back to matching by MAC as done today
> > >
> > > That should work for all virtio transports.
> >
> > True. I'm a bit unhappy that it's virtio net specific though
> > since down the road I expect we'll have a very similar feature
> > for scsi (and maybe others).
> >
> > But we do not have a way to have fields that are portable
> > both across devices and transports, and I think it would
> > be a useful addition. How would this work though? Any idea?
>
> Can we introduce some kind of device-independent config space area?
> Pushing back the device-specific config space by a certain value if the
> appropriate feature is negotiated and use that for things like the uuid?
So config moves back and forth?
Reminds me of the msi vector mess we had with pci.
I'd rather have every transport add a new config.
> But regardless of that, I'm not sure whether extending this approach to
> other device types is the way to go. Tying together two different
> devices is creating complicated situations at least in the hypervisor
> (even if it's fairly straightforward in the guest). [I have not come
> around again to look at the "how to handle visibility in QEMU"
> questions due to lack of cycles, sorry about that.]
>
> So, what's the goal of this approach? Only to allow migration with
> vfio-pci, or also to plug in a faster device and use it instead of an
> already attached paravirtualized device?
These are two sides of the same coin, I think the second approach
is closer to what we are doing here.
> What about migration of vfio devices that are not easily replaced by a
> paravirtualized device? I'm thinking of vfio-ccw, where our main (and
> currently only) supported device is dasd (disks) -- which can do a lot
> of specialized things that virtio-blk does not support (and should not
> or even cannot support).
But maybe virtio-scsi can?
> Would it be more helpful to focus on generic
> migration support for vfio instead of going about it device by device?
>
> This network device approach already seems far along, so it makes sense
> to continue with it. But I'm not sure whether we want to spend time and
> energy on that for other device types rather than working on a general
> solution for vfio migration.
I'm inclined to say finalizing this feature would be a good first step
and will teach us how we can move forward.
--
MST
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 20:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, Cornelia Huck, qemu-devel,
virtualization, Venu Busireddy, Netdev, boris.ostrovsky,
aaron.f.brown, Joao Martins
In-Reply-To: <20180622053141-mutt-send-email-mst@kernel.org>
On Thu, Jun 21, 2018 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jun 21, 2018 at 06:21:55PM -0700, Siwei Liu wrote:
>> On Thu, Jun 21, 2018 at 7:59 AM, Cornelia Huck <cohuck@redhat.com> wrote:
>> > On Wed, 20 Jun 2018 22:48:58 +0300
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >
>> >> On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
>> >> > In any case, I'm not sure anymore why we'd want the extra uuid.
>> >>
>> >> It's mostly so we can have e.g. multiple devices with same MAC
>> >> (which some people seem to want in order to then use
>> >> then with different containers).
>> >>
>> >> But it is also handy for when you assign a PF, since then you
>> >> can't set the MAC.
>> >>
>> >
>> > OK, so what about the following:
>> >
>> > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
>> > that we have a new uuid field in the virtio-net config space
>> > - in QEMU, add a property for virtio-net that allows to specify a uuid,
>> > offer VIRTIO_NET_F_STANDBY_UUID if set
>> > - when configuring, set the property to the group UUID of the vfio-pci
>> > device
>>
>> If feature negotiation fails on VIRTIO_NET_F_STANDBY_UUID, is it safe
>> to still expose UUID in the config space on virtio-pci?
>
>
> Yes but guest is not supposed to read it.
>
>> I'm not even sure if it's sane to expose group UUID on the PCI bridge
>> where the corresponding vfio-pci device attached to for a guest which
>> doesn't support the feature (legacy).
>>
>> -Siwei
>
> Yes but you won't add the primary behind such a bridge.
I assume the UUID feature is a new one besides the exiting
VIRTIO_NET_F_STANDBY feature, where I think the current proposal is,
if UUID feature is present and supported by guest, we'll do pairing
based on UUID; if UUID feature present or not supported by guest,
we'll still plug in the VF (if guest supports the _F_STANDBY feature)
but the pairing will be fallback to using MAC address. Are you saying
you don't even want to plug in the primary when the UUID feature is
not supported by guest? Does the feature negotiation UUID have to
depend on STANDBY being set, or the other way around? I thought that
just the absence of STANDBY will prevent primary to get exposed to the
guest.
-Siwei
>
>>
>> > - in the guest, use the uuid from the virtio-net device's config space
>> > if applicable; else, fall back to matching by MAC as done today
>> >
>> > That should work for all virtio transports.
^ permalink raw reply
* [PATCH] selftests: bpf: notification about privilege required to run test_lwt_seg6local.sh testing script
From: Jeffrin Jose T @ 2018-06-22 20:00 UTC (permalink / raw)
To: ast, daniel, shuah; +Cc: netdev, linux-kernel, linux-kselftest, Jeffrin Jose T
This test_lwt_seg6local.sh will execute with root privilege.
This patch is atleast used to notify the user about the privilege
required related to smooth execution of the script.
Signed-off-by: Jeffrin Jose T (Rajagiri SET) <ahiliation@gmail.com>
---
tools/testing/selftests/bpf/test_lwt_seg6local.sh | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_lwt_seg6local.sh b/tools/testing/selftests/bpf/test_lwt_seg6local.sh
index 1c77994b5e71..30575577a8b2 100755
--- a/tools/testing/selftests/bpf/test_lwt_seg6local.sh
+++ b/tools/testing/selftests/bpf/test_lwt_seg6local.sh
@@ -21,6 +21,15 @@
# An UDP datagram is sent from fb00::1 to fb00::6. The test succeeds if this
# datagram can be read on NS6 when binding to fb00::6.
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+msg="skip all tests:"
+if [ $UID != 0 ]; then
+ echo $msg please run this as root >&2
+ exit $ksft_skip
+fi
+
TMP_FILE="/tmp/selftest_lwt_seg6local.txt"
cleanup()
--
2.17.0
^ permalink raw reply related
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 20:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, Cornelia Huck, qemu-devel,
virtualization, Venu Busireddy, Netdev, boris.ostrovsky,
aaron.f.brown, Joao Martins
In-Reply-To: <CADGSJ23SBEFJCD50K-F-Gt86Q1-i_6iHTmSSjdXDzNnnpWZ_oQ@mail.gmail.com>
On Fri, Jun 22, 2018 at 1:00 PM, Siwei Liu <loseweigh@gmail.com> wrote:
> On Thu, Jun 21, 2018 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, Jun 21, 2018 at 06:21:55PM -0700, Siwei Liu wrote:
>>> On Thu, Jun 21, 2018 at 7:59 AM, Cornelia Huck <cohuck@redhat.com> wrote:
>>> > On Wed, 20 Jun 2018 22:48:58 +0300
>>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> >
>>> >> On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
>>> >> > In any case, I'm not sure anymore why we'd want the extra uuid.
>>> >>
>>> >> It's mostly so we can have e.g. multiple devices with same MAC
>>> >> (which some people seem to want in order to then use
>>> >> then with different containers).
>>> >>
>>> >> But it is also handy for when you assign a PF, since then you
>>> >> can't set the MAC.
>>> >>
>>> >
>>> > OK, so what about the following:
>>> >
>>> > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
>>> > that we have a new uuid field in the virtio-net config space
>>> > - in QEMU, add a property for virtio-net that allows to specify a uuid,
>>> > offer VIRTIO_NET_F_STANDBY_UUID if set
>>> > - when configuring, set the property to the group UUID of the vfio-pci
>>> > device
>>>
>>> If feature negotiation fails on VIRTIO_NET_F_STANDBY_UUID, is it safe
>>> to still expose UUID in the config space on virtio-pci?
>>
>>
>> Yes but guest is not supposed to read it.
>>
>>> I'm not even sure if it's sane to expose group UUID on the PCI bridge
>>> where the corresponding vfio-pci device attached to for a guest which
>>> doesn't support the feature (legacy).
>>>
>>> -Siwei
>>
>> Yes but you won't add the primary behind such a bridge.
>
> I assume the UUID feature is a new one besides the exiting
> VIRTIO_NET_F_STANDBY feature, where I think the current proposal is,
> if UUID feature is present and supported by guest, we'll do pairing
> based on UUID; if UUID feature present
^^^^^^^ is NOT present
> or not supported by guest,
> we'll still plug in the VF (if guest supports the _F_STANDBY feature)
> but the pairing will be fallback to using MAC address. Are you saying
> you don't even want to plug in the primary when the UUID feature is
> not supported by guest? Does the feature negotiation UUID have to
> depend on STANDBY being set, or the other way around? I thought that
> just the absence of STANDBY will prevent primary to get exposed to the
> guest.
>
> -Siwei
>
>>
>>>
>>> > - in the guest, use the uuid from the virtio-net device's config space
>>> > if applicable; else, fall back to matching by MAC as done today
>>> >
>>> > That should work for all virtio transports.
^ permalink raw reply
* Re: Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 20:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cornelia Huck, Samudrala, Sridhar, Alexander Duyck, virtio-dev,
aaron.f.brown, Jiri Pirko, Jakub Kicinski, Netdev, qemu-devel,
virtualization, konrad.wilk, boris.ostrovsky, Joao Martins,
Venu Busireddy, vijay.balakrishna
In-Reply-To: <20180622214259-mutt-send-email-mst@kernel.org>
On Fri, Jun 22, 2018 at 12:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jun 22, 2018 at 05:09:55PM +0200, Cornelia Huck wrote:
>> On Thu, 21 Jun 2018 21:20:13 +0300
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>> > On Thu, Jun 21, 2018 at 04:59:13PM +0200, Cornelia Huck wrote:
>> > > OK, so what about the following:
>> > >
>> > > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
>> > > that we have a new uuid field in the virtio-net config space
>> > > - in QEMU, add a property for virtio-net that allows to specify a uuid,
>> > > offer VIRTIO_NET_F_STANDBY_UUID if set
>> > > - when configuring, set the property to the group UUID of the vfio-pci
>> > > device
>> > > - in the guest, use the uuid from the virtio-net device's config space
>> > > if applicable; else, fall back to matching by MAC as done today
>> > >
>> > > That should work for all virtio transports.
>> >
>> > True. I'm a bit unhappy that it's virtio net specific though
>> > since down the road I expect we'll have a very similar feature
>> > for scsi (and maybe others).
>> >
>> > But we do not have a way to have fields that are portable
>> > both across devices and transports, and I think it would
>> > be a useful addition. How would this work though? Any idea?
>>
>> Can we introduce some kind of device-independent config space area?
>> Pushing back the device-specific config space by a certain value if the
>> appropriate feature is negotiated and use that for things like the uuid?
>
> So config moves back and forth?
> Reminds me of the msi vector mess we had with pci.
> I'd rather have every transport add a new config.
>
>> But regardless of that, I'm not sure whether extending this approach to
>> other device types is the way to go. Tying together two different
>> devices is creating complicated situations at least in the hypervisor
>> (even if it's fairly straightforward in the guest). [I have not come
>> around again to look at the "how to handle visibility in QEMU"
>> questions due to lack of cycles, sorry about that.]
>>
>> So, what's the goal of this approach? Only to allow migration with
>> vfio-pci, or also to plug in a faster device and use it instead of an
>> already attached paravirtualized device?
>
> These are two sides of the same coin, I think the second approach
> is closer to what we are doing here.
I'm leaning towards being conservative to keep the potential of doing
both. It's the vfio migration itself that has to be addessed, not to
make every virtio device to have an accelerated datapath, right?
-Siwei
>
>> What about migration of vfio devices that are not easily replaced by a
>> paravirtualized device? I'm thinking of vfio-ccw, where our main (and
>> currently only) supported device is dasd (disks) -- which can do a lot
>> of specialized things that virtio-blk does not support (and should not
>> or even cannot support).
>
> But maybe virtio-scsi can?
>
>> Would it be more helpful to focus on generic
>> migration support for vfio instead of going about it device by device?
>>
>> This network device approach already seems far along, so it makes sense
>> to continue with it. But I'm not sure whether we want to spend time and
>> energy on that for other device types rather than working on a general
>> solution for vfio migration.
>
> I'm inclined to say finalizing this feature would be a good first step
> and will teach us how we can move forward.
>
> --
> MST
^ permalink raw reply
* Re: [PATCH 0/4] docs: e100[0] fix build errors
From: Randy Dunlap @ 2018-06-22 20:22 UTC (permalink / raw)
To: Tobin C. Harding, Jonathan Corbet
Cc: Jeff Kirsher, David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <20180622003708.31848-1-me@tobin.cc>
Hi Tobin,
On 06/21/2018 05:37 PM, Tobin C. Harding wrote:
> Hi Jonathan,
>
> This patch set fixes current docs build failure on Linus' mainline
>
> commit: (ba4dbdedd3ed Merge tag 'jfs-4.18' of git://github.com/kleikamp/linux-shaggy)
>
> (FYI this is 8 commits after Linux 4.18-rc1).
>
> And also same build errors on today's linux-next
>
> 8439c34f07a3 (tag: next-20180621, linux-next/master, linux-next) Add linux-next specific files for 20180621
>
>
> I split the patches in between the two drivers to enable use of the
> 'Fixes:' tag.
>
> Tobin C. Harding (4):
> Documentation: e100: Use correct heading adornment
> Documentation: e1000: Use correct heading adornment
> Documentation: e100: Fix docs build error
> Documentation: e1000: Fix docs build error
>
> Documentation/networking/e100.rst | 112 +++++++++++++++--------------
> Documentation/networking/e1000.rst | 76 ++++++++++----------
> 2 files changed, 96 insertions(+), 92 deletions(-)
I am still seeing a few warnings (your 4 patches applied to
linux-next-20180622):
linux-next-20180622/Documentation/networking/e100.rst:57: WARNING: Literal block expected; none found.
linux-next-20180622/Documentation/networking/e100.rst:68: WARNING: Literal block expected; none found.
linux-next-20180622/Documentation/networking/e100.rst:75: WARNING: Literal block expected; none found.
linux-next-20180622/Documentation/networking/e100.rst:84: WARNING: Literal block expected; none found.
linux-next-20180622/Documentation/networking/e100.rst:93: WARNING: Inline emphasis start-string without end-string.
linux-next-20180622/Documentation/networking/e1000.rst:83: ERROR: Unexpected indentation.
linux-next-20180622/Documentation/networking/e1000.rst:84: WARNING: Block quote ends without a blank line; unexpected unindent.
linux-next-20180622/Documentation/networking/e1000.rst:173: WARNING: Definition list ends without a blank line; unexpected unindent.
linux-next-20180622/Documentation/networking/e1000.rst:236: WARNING: Definition list ends without a blank line; unexpected unindent.
You didn't get these warnings? or they weren't important?
or "perfect" was not your primary goal? :) [which is fine]
[I see around 50 similar doc formatting warnings/errors in the entire
Documentation build.]
Anyway, much better than it was. Thanks.
Tested-by: Randy Dunlap <rdunlap@infradead.org>
--
~Randy
^ permalink raw reply
* [PATCH net] net: mvneta: fix the Rx desc DMA address in the Rx path
From: Yan Markman @ 2018-06-22 20:26 UTC (permalink / raw)
To: Antoine Tenart, davem@davemloft.net, gregory.clement@bootlin.com,
mw@semihalf.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
thomas.petazzoni@bootlin.com, maxime.chevallier@bootlin.com,
miquel.raynal@bootlin.com, Nadav Haklai, Stefan Chulski,
Yelena Krivosheev
YANM: adding Yelena-K
--------------------------------
When using s/w buffer management, buffers are allocated and DMA mapped.
When doing so on an arm64 platform, an offset correction is applied on the DMA address, before storing it in an Rx descriptor. The issue is this DMA address is then used later in the Rx path without removing the offset correction. Thus the DMA address is wrong, which can led to various issues.
This patch fixes this by removing the offset correction from the DMA address retrieved from the Rx descriptor before using it in the Rx path.
Fixes: 8d5047cf9ca2 ("net: mvneta: Convert to be 64 bits compatible")
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 17a904cc6a5e..0ad2f3f7da85 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1932,7 +1932,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo,
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
index = rx_desc - rxq->descs;
data = rxq->buf_virt_addr[index];
- phys_addr = rx_desc->buf_phys_addr;
+ phys_addr = rx_desc->buf_phys_addr - pp->rx_offset_correction;
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
--
2.17.1
^ permalink raw reply related
* offload_handle issue in ipsec offload
From: Shannon Nelson @ 2018-06-22 20:30 UTC (permalink / raw)
To: Steffen Klassert
Cc: netdev@vger.kernel.org, Boris Pismenny, Ilan Tayari, Don Skidmore
Hi Steffen,
While adding the ipsec-offload API to netdevsim I ran across an issue
with the use of x->xso.offload_handle that I think needs attention, and
would like your opinion before I try to address it.
The offload_handle is essentially an opaque magic cookie to be used by
the driver to help track the driver's offload information. As such, the
driver fills it in when an SA is setup for an offload, and then the
offload_handle value can be used in the driver's xmit handler to quickly
find the device specific config information for setting up each packet.
Since this is driver specific information the stack code, e.g.
xfrm_dev_offload_ok() and validate_xmit_xfrm() and a couple of esp
functions, really shouldn't be trying to use it. However, since they
are checking offload_handle for == 0 to see if it has been used, the
drivers need to be sure they have set it to non-zero. This requirement
is not clear in the API description at the moment.
I ran across this because my addition to netdevsim uses offload_handle
to store the index of the array item that has the related SA
information. I found in testing that the Tx offload wasn't getting
called because the array index was 0 and so xfrm_dev_offload_ok() would
think there was no offload and bypass the offloaded Tx. It turns out
that the ixgbe implementation has the same bug, but I missed this in
earlier testing. The Mellanox driver doesn't run into this problem
because they are storing a struct pointer rather than an array index.
Does the stack really need to check offload_handle? I don't think it
does, but perhaps I'm missing something. Here's where I found it used:
- validate_xmit_xfrm() - added in 3dca3f38
- xfrm_dev_offload_ok() - added in original patch d77e38e6
- esp4_gso_segment() - added in 3dca3f38
- esp_xmit() - added in fca11ebd
- esp6_gso_segment() - added in 3dca3f38
- esp6_xmit() - added in 3dca3f38
I think in all cases it is unnecessary and could be pulled out.
If these checks need to be left in, then any client drivers need to be
sure this is a non-zero value, possibly by adding a VALID bit to their
opaque value. The ixgbe and netdevsim implementations would be fine
with using a high bit as a VALID flag, but that might not play well with
drivers like Mellanox that store a struct address.
So, after all that... shall we remove the offload_handle references in
the stack code?
sln
--
==================================================
Shannon Nelson shannon.nelson@oracle.com
Parents can't afford to be squeamish
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Martin KaFai Lau @ 2018-06-22 20:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov, Yonghong Song,
Quentin Monnet, David S. Miller, netdev, kernel-team,
linux-kernel
In-Reply-To: <20180622114032.162b2a76@cakuba.netronome.com>
On Fri, Jun 22, 2018 at 11:40:32AM -0700, Jakub Kicinski wrote:
> On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> > On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > > $ sudo bpftool map dump -p id 14
> > > > > > > > [{
> > > > > > > > "key": 0
> > > > > > > > },{
> > > > > > > > "value": {
> > > > > > > > "m": 1,
> > > > > > > > "n": 2,
> > > > > > > > "o": "c",
> > > > > > > > "p": [15,16,17,18,15,16,17,18
> > > > > > > > ],
> > > > > > > > "q": [[25,26,27,28,25,26,27,28
> > > > > > > > ],[35,36,37,38,35,36,37,38
> > > > > > > > ],[45,46,47,48,45,46,47,48
> > > > > > > > ],[55,56,57,58,55,56,57,58
> > > > > > > > ]
> > > > > > > > ],
> > > > > > > > "r": 1,
> > > > > > > > "s": 0x7ffff6f70568,
> > > > > > > > "t": {
> > > > > > > > "x": 5,
> > > > > > > > "y": 10
> > > > > > > > },
> > > > > > > > "u": 100,
> > > > > > > > "v": 20,
> > > > > > > > "w1": 0x7,
> > > > > > > > "w2": 0x3
> > > > > > > > }
> > > > > > > > }
> > > > > > > > ]
> > > > > > >
> > > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > > JSON must remain backwards compatible.
> > > > > > >
> > > > > > > The dump today has object per entry, e.g.:
> > > > > > >
> > > > > > > {
> > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > ],
> > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > ]
> > > > > > > }
> > > > > > >
> > > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > > >
> > > > > > > {
> > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > ],
> > > > > > > "key_struct":{
> > > > > > > "index":0
> > > > > > > },
> > Got a few questions.
> >
> > When we support hashtab later, the key could be int
> > but reusing the name as "index" is weird.
>
> Ugh, yes, naturally. I just typed that out without thinking, so for
> array maps there is usually no BTF info?... For hashes obviously we
The key of array map also has BTF info which must be an int.
> should just use the BTF, I'm not sure we should format indexes for
> arrays nicely or not :S
>
> > The key could also be a struct (e.g. a struct to describe ip:port).
> > Can you suggest how the "key_struct" will look like?
>
> Hm. I think in my mind it has only been a struct but that's not true :S
> So the struct in the name makes very limited sense now.
>
> Should we do:
> "formatted" : {
> "value" : XXX
> }
>
> Where
> XXX == plain int for integers, e.g. "value":0
> XXX == array for arrays, e.g. "value":[1,2,3,4]
> XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
It is exactly how this patch is using json's {}, [] and int. ;)
but other than that, it does not have to be json.
In the next spin, lets stop calling this output json to avoid wrong
user's expection and I also don't want the future readability
improvements to be limited by that. Lets call it something
like plain text output with BTF.
How about:
When "bpftool map dump id 1" is used, it will print the BTF plaintext output
if a map has BTF available. If not, it will print the existing
plaintext output. That should solve the concern about the user may not
know BTF is available.
This ascii output is for human. The script should not parse the ascii output
because it is silly not to use the binary ABI (like what this patch is using)
which does not suffer backward compat issue.
The existing "-j" can be used to dump the map's binary data
for remote debugging purpose. The BTF is in one of the elf section of
the bpf_prog.o.
>
> > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > ],
> > > > > > > "value_struct":{
> > > > > > > "src_ip":2,
> > If for the same map the user changes the "src_ip" to an array of int[4]
> > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > Is it breaking backward compat?
> > i.e.
> > struct five_tuples {
> > - int src_ip;
> > + int src_ip[4];
> > /* ... */
> > };
>
> Well, it is breaking backward compat, but it's the program doing it,
> not bpftool :) BTF changes so does the output.
As we see, the key/value's btf-output is inherently not backward compat.
Hence, "-j" and "-p" will stay as is. The whole existing json will
be backward compat instead of only partly backward compat.
>
> > > > > > > "dst_ip:0
> > > > > > > }
> > > > > > > }
> > > > > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > > > > while most people would prefer "key_struct"/"value_struct" if it is
> > > > > > available.
> > > > >
> > > > > Agreed, it's not that useful, especially with the string-hex debacle :(
> > > > > It's just about the backwards compat.
> > > > >
> > > > > > How about introducing a new option, like "-b", to print the
> > > > > > map with BTF (if available) such that it won't break the existing
> > > > > > one (-j or -p) while the "-b" output can keep using the "key"
> > > > > > and "value".
> > > > > >
> > > > > > The existing json can be kept as is.
> > > > >
> > > > > That was my knee jerk reaction too, but on reflection it doesn't sound
> > > > > that great. We expect people with new-enough bpftool to use btf, so it
> > > > > should be available in the default output, without hiding it behind a
> > > > > switch. We could add a switch to hide the old output, but that doesn't
> > > > > give us back the names... What about Key and Value or k and v? Or
> > > > > key_fields and value_fields?
> > > > I thought the current default output is "plain" ;)
> > > > Having said that, yes, the btf is currently printed in json.
> > > >
> > > > Ideally, the default json output should do what most people want:
> > > > print btf and btf only (if it is available).
> > > > but I don't see a way out without new option if we need to
> > > > be backward compat :(
> > > >
> > > > Agree that showing the btf in the existing json output will be useful (e.g.
> > > > to hint people that BTF is available). If btf is showing in old json,
> > > > also agree that the names should be the same with the new json.
> > > > key_fields and value_fields may hint it has >1 fields though.
> > > > May be "formatted_key" and "formatted_value"?
> > >
> > > SGTM! Or even maybe as a "formatted" object?:
> > >
> > > {
> > > "key":["0x00","0x00","0x00","0x00",
> > > ],
> > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > ],
> > > "formatted":{
> > > "key":{
> > > "index":0
> > > },
> > > "value":{
> > > "src_ip":2,
> > > "dst_ip:0
> > > }
> > > }
> > hmm... that is an extra indentation (keep in mind that the "value" could
> > already have a few nested structs which itself consumes a few indentations)
> > but I guess adding another one may be ok-ish.
>
> I'm not fussed about this, whatever JSON writer does by default is fine
> with me, really.
>
> > > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > > better one?
> > > > > > >
> > > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > > >
> > > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > > bpftool patches before posting.
> > > > > > >
> > > > > > > Thanks for working on this!
> > >
>
^ permalink raw reply
* [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()
From: Paul Moore @ 2018-06-22 21:18 UTC (permalink / raw)
To: netdev; +Cc: selinux, linux-security-module
From: Paul Moore <paul@paul-moore.com>
The ipv6_renew_options_kern() function eventually called into
copy_from_user(), despite it not using any userspace buffers, which
was problematic as that ended up calling access_ok() which emited
a warning on x86 (and likely other arches as well).
ipv6_renew_options_kern()
ipv6_renew_options()
ipv6_renew_option()
copy_from_user()
_copy_from_user()
access_ok()
The access_ok() check inside _copy_from_user() is obviously the right
thing to do which means that calling copy_from_user() via
ipv6_renew_options_kern() is obviously the wrong thing to do. This
patch fixes this by duplicating ipv6_renew_option() in the _kern()
variant, omitting the userspace copies and attributes. The patch
does make an attempt at limiting the duplicated code by moving the
option allocation code into a common helper function. I'm not in
love with this solution, but everything else I could think of seemed
worse.
The ipv6_renew_options_kern() function is an required by the
CALIPSO/RFC5570 code in net/ipv6/calipso.c.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
net/ipv6/exthdrs.c | 155 +++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 121 insertions(+), 34 deletions(-)
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5bc2bf3733ab..902748acd6fe 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1040,36 +1040,47 @@ static int ipv6_renew_option(void *ohdr,
return 0;
}
+static int ipv6_renew_option_kern(void *ohdr,
+ struct ipv6_opt_hdr *newopt, int newoptlen,
+ int inherit,
+ struct ipv6_opt_hdr **hdr,
+ char **p)
+{
+ if (inherit) {
+ if (ohdr) {
+ memcpy(*p, ohdr,
+ ipv6_optlen((struct ipv6_opt_hdr *)ohdr));
+ *hdr = (struct ipv6_opt_hdr *)*p;
+ *p += CMSG_ALIGN(ipv6_optlen(*hdr));
+ }
+ } else if (newopt) {
+ memcpy(*p, newopt, newoptlen);
+ *hdr = (struct ipv6_opt_hdr *)*p;
+ if (ipv6_optlen(*hdr) > newoptlen)
+ return -EINVAL;
+ *p += CMSG_ALIGN(newoptlen);
+ }
+ return 0;
+}
+
/**
- * ipv6_renew_options - replace a specific ext hdr with a new one.
+ * ipv6_renew_option_alloc - helper function for allocating ipv6_txoptions
*
* @sk: sock from which to allocate memory
* @opt: original options
* @newtype: option type to replace in @opt
- * @newopt: new option of type @newtype to replace (user-mem)
- * @newoptlen: length of @newopt
- *
- * Returns a new set of options which is a copy of @opt with the
- * option type @newtype replaced with @newopt.
+ * @newoptlen: length of the new option
*
- * @opt may be NULL, in which case a new set of options is returned
- * containing just @newopt.
- *
- * @newopt may be NULL, in which case the specified option type is
- * not copied into the new set of options.
- *
- * The new set of options is allocated from the socket option memory
- * buffer of @sk.
+ * This really should only ever be called by ipv6_renew_option() or
+ * ipv6_renew_option_kern().
*/
-struct ipv6_txoptions *
-ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr __user *newopt, int newoptlen)
+static struct ipv6_txoptions *ipv6_renew_option_alloc(struct sock *sk,
+ struct ipv6_txoptions *opt,
+ int newtype,
+ int newoptlen)
{
int tot_len = 0;
- char *p;
struct ipv6_txoptions *opt2;
- int err;
if (opt) {
if (newtype != IPV6_HOPOPTS && opt->hopopt)
@@ -1082,7 +1093,7 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
tot_len += CMSG_ALIGN(ipv6_optlen(opt->dst1opt));
}
- if (newopt && newoptlen)
+ if (newoptlen)
tot_len += CMSG_ALIGN(newoptlen);
if (!tot_len)
@@ -1096,6 +1107,44 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
memset(opt2, 0, tot_len);
refcount_set(&opt2->refcnt, 1);
opt2->tot_len = tot_len;
+
+ return opt2;
+}
+
+/**
+ * ipv6_renew_options - replace a specific ext hdr with a new one.
+ *
+ * @sk: sock from which to allocate memory
+ * @opt: original options
+ * @newtype: option type to replace in @opt
+ * @newopt: new option of type @newtype to replace (user-mem)
+ * @newoptlen: length of @newopt
+ *
+ * Returns a new set of options which is a copy of @opt with the
+ * option type @newtype replaced with @newopt.
+ *
+ * @opt may be NULL, in which case a new set of options is returned
+ * containing just @newopt.
+ *
+ * @newopt may be NULL, in which case the specified option type is
+ * not copied into the new set of options.
+ *
+ * The new set of options is allocated from the socket option memory
+ * buffer of @sk.
+ */
+struct ipv6_txoptions *
+ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
+ int newtype,
+ struct ipv6_opt_hdr __user *newopt, int newoptlen)
+{
+ char *p;
+ struct ipv6_txoptions *opt2;
+ int err;
+
+ opt2 = ipv6_renew_option_alloc(sk, opt, newtype,
+ newopt && newoptlen ? newoptlen : 0);
+ if (!opt2 || IS_ERR(opt2))
+ return opt2;
p = (char *)(opt2 + 1);
err = ipv6_renew_option(opt ? opt->hopopt : NULL, newopt, newoptlen,
@@ -1142,23 +1191,61 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
* @newopt: new option of type @newtype to replace (kernel-mem)
* @newoptlen: length of @newopt
*
- * See ipv6_renew_options(). The difference is that @newopt is
- * kernel memory, rather than user memory.
+ * See ipv6_renew_options(). The difference is that @newopt is kernel memory,
+ * rather than user memory.
*/
struct ipv6_txoptions *
ipv6_renew_options_kern(struct sock *sk, struct ipv6_txoptions *opt,
- int newtype, struct ipv6_opt_hdr *newopt,
- int newoptlen)
+ int newtype,
+ struct ipv6_opt_hdr *newopt, int newoptlen)
{
- struct ipv6_txoptions *ret_val;
- const mm_segment_t old_fs = get_fs();
-
- set_fs(KERNEL_DS);
- ret_val = ipv6_renew_options(sk, opt, newtype,
- (struct ipv6_opt_hdr __user *)newopt,
- newoptlen);
- set_fs(old_fs);
- return ret_val;
+ char *p;
+ struct ipv6_txoptions *opt2;
+ int err;
+
+ opt2 = ipv6_renew_option_alloc(sk, opt, newtype,
+ newopt && newoptlen ? newoptlen : 0);
+ if (!opt2 || IS_ERR(opt2))
+ return opt2;
+ p = (char *)(opt2 + 1);
+
+ err = ipv6_renew_option_kern(opt ? opt->hopopt : NULL,
+ newopt, newoptlen,
+ newtype != IPV6_HOPOPTS,
+ &opt2->hopopt, &p);
+ if (err)
+ goto out;
+
+ err = ipv6_renew_option_kern(opt ? opt->dst0opt : NULL,
+ newopt, newoptlen,
+ newtype != IPV6_RTHDRDSTOPTS,
+ &opt2->dst0opt, &p);
+ if (err)
+ goto out;
+
+ err = ipv6_renew_option_kern(opt ? opt->srcrt : NULL,
+ newopt, newoptlen,
+ newtype != IPV6_RTHDR,
+ (struct ipv6_opt_hdr **)&opt2->srcrt, &p);
+ if (err)
+ goto out;
+
+ err = ipv6_renew_option_kern(opt ? opt->dst1opt : NULL,
+ newopt, newoptlen,
+ newtype != IPV6_DSTOPTS,
+ &opt2->dst1opt, &p);
+ if (err)
+ goto out;
+
+ opt2->opt_nflen = (opt2->hopopt ? ipv6_optlen(opt2->hopopt) : 0) +
+ (opt2->dst0opt ? ipv6_optlen(opt2->dst0opt) : 0) +
+ (opt2->srcrt ? ipv6_optlen(opt2->srcrt) : 0);
+ opt2->opt_flen = (opt2->dst1opt ? ipv6_optlen(opt2->dst1opt) : 0);
+
+ return opt2;
+out:
+ sock_kfree_s(sk, opt2, opt2->tot_len);
+ return ERR_PTR(err);
}
struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
^ permalink raw reply related
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Jakub Kicinski @ 2018-06-22 21:27 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov, Yonghong Song,
Quentin Monnet, David S. Miller, netdev, kernel-team,
linux-kernel
In-Reply-To: <20180622205535.c6vjhdwt5er4wc32@kafai-mbp.dhcp.thefacebook.com>
On Fri, 22 Jun 2018 13:58:52 -0700, Martin KaFai Lau wrote:
> On Fri, Jun 22, 2018 at 11:40:32AM -0700, Jakub Kicinski wrote:
> > On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> > > On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > > > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > > > $ sudo bpftool map dump -p id 14
> > > > > > > > > [{
> > > > > > > > > "key": 0
> > > > > > > > > },{
> > > > > > > > > "value": {
> > > > > > > > > "m": 1,
> > > > > > > > > "n": 2,
> > > > > > > > > "o": "c",
> > > > > > > > > "p": [15,16,17,18,15,16,17,18
> > > > > > > > > ],
> > > > > > > > > "q": [[25,26,27,28,25,26,27,28
> > > > > > > > > ],[35,36,37,38,35,36,37,38
> > > > > > > > > ],[45,46,47,48,45,46,47,48
> > > > > > > > > ],[55,56,57,58,55,56,57,58
> > > > > > > > > ]
> > > > > > > > > ],
> > > > > > > > > "r": 1,
> > > > > > > > > "s": 0x7ffff6f70568,
> > > > > > > > > "t": {
> > > > > > > > > "x": 5,
> > > > > > > > > "y": 10
> > > > > > > > > },
> > > > > > > > > "u": 100,
> > > > > > > > > "v": 20,
> > > > > > > > > "w1": 0x7,
> > > > > > > > > "w2": 0x3
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > ]
> > > > > > > >
> > > > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > > > JSON must remain backwards compatible.
> > > > > > > >
> > > > > > > > The dump today has object per entry, e.g.:
> > > > > > > >
> > > > > > > > {
> > > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > > ],
> > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > ]
> > > > > > > > }
> > > > > > > >
> > > > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > > > >
> > > > > > > > {
> > > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > > ],
> > > > > > > > "key_struct":{
> > > > > > > > "index":0
> > > > > > > > },
> > > Got a few questions.
> > >
> > > When we support hashtab later, the key could be int
> > > but reusing the name as "index" is weird.
> >
> > Ugh, yes, naturally. I just typed that out without thinking, so for
> > array maps there is usually no BTF info?... For hashes obviously we
> The key of array map also has BTF info which must be an int.
Perfect.
> > should just use the BTF, I'm not sure we should format indexes for
> > arrays nicely or not :S
> >
> > > The key could also be a struct (e.g. a struct to describe ip:port).
> > > Can you suggest how the "key_struct" will look like?
> >
> > Hm. I think in my mind it has only been a struct but that's not true :S
> > So the struct in the name makes very limited sense now.
> >
> > Should we do:
> > "formatted" : {
> > "value" : XXX
> > }
> >
> > Where
> > XXX == plain int for integers, e.g. "value":0
> > XXX == array for arrays, e.g. "value":[1,2,3,4]
> > XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
> It is exactly how this patch is using json's {}, [] and int. ;)
Great, then just wrap that in a "formatted" object instead of
redefining fields and we're good?
> but other than that, it does not have to be json.
> In the next spin, lets stop calling this output json to avoid wrong
> user's expection and I also don't want the future readability
> improvements to be limited by that. Lets call it something
> like plain text output with BTF.
I don't understand. We are discussing JSON output here. The example we
are commenting on is output of:
$ sudo bpftool map dump -p id 14
That -p means JSON. Nobody objects to plain test output changes. I
actually didn't realize you haven't implemented plain text in this
series, we should have both.
> How about:
> When "bpftool map dump id 1" is used, it will print the BTF plaintext output
> if a map has BTF available. If not, it will print the existing
> plaintext output. That should solve the concern about the user may not
> know BTF is available.
>
> This ascii output is for human. The script should not parse the ascii output
> because it is silly not to use the binary ABI (like what this patch is using)
> which does not suffer backward compat issue.
What binary ABI? I'm also not 100% sure what this patch is doing as it
adds bunch of code in new files that never gets called:
tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
tools/bpf/bpftool/btf_dumper.h | 18 ++
2 files changed, 265 insertions(+)
> The existing "-j" can be used to dump the map's binary data
> for remote debugging purpose. The BTF is in one of the elf section of
> the bpf_prog.o.
> > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > ],
> > > > > > > > "value_struct":{
> > > > > > > > "src_ip":2,
> > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > Is it breaking backward compat?
> > > i.e.
> > > struct five_tuples {
> > > - int src_ip;
> > > + int src_ip[4];
> > > /* ... */
> > > };
> >
> > Well, it is breaking backward compat, but it's the program doing it,
> > not bpftool :) BTF changes so does the output.
> As we see, the key/value's btf-output is inherently not backward compat.
> Hence, "-j" and "-p" will stay as is. The whole existing json will
> be backward compat instead of only partly backward compat.
No. There is a difference between user of a facility changing their
input and kernel/libraries providing different output in response to
that, and the libraries suddenly changing the output on their own.
Your example is like saying if user started using IPv6 addresses
instead of IPv4 the netlink attributes in dumps will be different so
kernel didn't keep backwards compat. While what you're doing is more
equivalent to dropping support for old ioctl interfaces because there
is a better mechanism now.
BTF in JSON is very useful, and will help people who writes simple
orchestration/scripts based on bpftool *a* *lot*. I really appreciate
this addition to bpftool and will start using it myself as soon as it
lands. I'm not sure why the reluctance to slightly change the output
format?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox