* Re: [PATCH] net: ethernet: mediatek: Fix overlapping capability bits.
From: Willem de Bruijn @ 2019-06-29 19:33 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, f.fainelli, linux, David Miller, matthias.bgg, andrew,
vivien.didelot, frank-w, Network Development, linux-mediatek,
linux-mips
In-Reply-To: <20190629122419.19026-1-opensource@vdorst.com>
On Sat, Jun 29, 2019 at 8:24 AM René van Dorst <opensource@vdorst.com> wrote:
>
> Both MTK_TRGMII_MT7621_CLK and MTK_PATH_BIT are defined as bit 10.
>
> This causes issues on non-MT7621 devices which has the
> MTK_PATH_BIT(MTK_ETH_PATH_GMAC1_RGMII) capability set.
> The wrong TRGMII setup code is executed.
>
> Moving the MTK_PATH_BIT to bit 11 fixes the issue.
>
> Fixes: 8efaa653a8a5 ("net: ethernet: mediatek: Add MT7621 TRGMII mode
> support")
> Signed-off-by: René van Dorst <opensource@vdorst.com>
This targets net? Please mark networking patches [PATCH net] or [PATCH
net-next].
> ---
> drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index 876ce6798709..2cb8a915731c 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -626,7 +626,7 @@ enum mtk_eth_path {
> #define MTK_TRGMII_MT7621_CLK BIT(10)
>
> /* Supported path present on SoCs */
> -#define MTK_PATH_BIT(x) BIT((x) + 10)
>
> +#define MTK_PATH_BIT(x) BIT((x) + 11)
>
To avoid this happening again, perhaps make the reserved range more explicit?
For instance
#define MTK_FIXED_BIT_LAST 10
#define MTK_TRGMII_MT7621_CLK BIT(MTK_FIXED_BIT_LAST)
#define MTK_PATH_BIT_FIRST (MTK_FIXED_BIT_LAST + 1)
#define MTK_PATH_BIT_LAST (MTK_FIXED_BIT_LAST + 7)
#define MTK_MUX_BIT_FIRST (MTK_PATH_BIT_LAST + 1)
Though I imagine there are cleaner approaches. Perhaps define all
fields as enum instead of just mtk_eth_mux and mtk_eth_path. Then
there can be no accidental collision.
^ permalink raw reply
* Re: net: check before dereferencing netdev_ops during busy poll
From: Matteo Croce @ 2019-06-29 19:39 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Josh Elsasser, Sasha Levin, stable, netdev, LKML, David Miller
In-Reply-To: <20190629074553.GA28708@kroah.com>
On Sat, Jun 29, 2019 at 9:45 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jun 28, 2019 at 07:03:01PM -0700, Josh Elsasser wrote:
> > On Jun 28, 2019, at 3:55 PM, Sasha Levin <sashal@kernel.org> wrote:
> >
> > > What's the upstream commit id?
> >
> > The commit wasn't needed upstream, as I only sent the original patch after
> > 79e7fff47b7b ("net: remove support for per driver ndo_busy_poll()") had
> > made the fix unnecessary in Linus' tree.
> >
> > May've gotten lost in the shuffle due to my poor Fixes tags. The patch in
> > question applied only on top of the 4.9 stable release at the time, but the
> > actual NPE had been around in some form since 3.11 / 0602129286705 ("net: add
> > low latency socket poll").
>
> Ok, can people then resend this and be very explicit as to why this is
> needed only in a stable kernel tree and get reviews from people agreeing
> that this really is the correct fix?
>
> thanks,
>
> greg k-h
Hi Greg,
I think that David alredy reviewed the patch here:
https://lore.kernel.org/netdev/20180313.105115.682846171057663636.davem@davemloft.net/
Anyway, I tested the patch and it fixes the panic, at least on my
iwlwifi card, so:
Tested-by: Matteo Croce <mcroce@redhat.com>
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* r8169 not working on 5.2.0rc6 with GPD MicroPC
From: Karsten Wiborg @ 2019-06-29 20:34 UTC (permalink / raw)
To: nic_swsd, romieu; +Cc: netdev
Hello,
writing to you because of the r8168-dkms-README.Debian.
I am using a GPD MicroPC running Debian Buster with Kernel:
Linux praktifix 5.2.0-050200rc6-generic #201906222033 SMP Sun Jun 23
00:36:46 UTC 2019 x86_64 GNU/Linux
Got the Kernel from:
https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.2-rc6/
Reason for the Kernel: it includes Hans DeGoedes necessary fixes for the
GPD MicroPC, see:
https://github.com/jwrdegoede/linux-sunxi
(btw. I also tried Hans' 5.2.0rc5-kernel which also did not work with
respect to Ethernet). Googling of course didn't help out either.
My GPD MicroPC with running r8169 gives the following lines in dmesg:
...
[ 2.839485] libphy: r8169: probed
[ 2.839776] r8169 0000:02:00.0 eth0: RTL8168h/8111h,
00:00:00:00:00:00, XID 541, IRQ 126
[ 2.839779] r8169 0000:02:00.0 eth0: jumbo features [frames: 9200
bytes, tx checksumming: ko]
...
[ 2.897924] r8169 0000:02:00.0 eno1: renamed from eth0
ip addr show gives me:
# ip addr show
...
2: eno1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group
default qlen 1000
link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
...
ethtool gives me:
# ethtool -i eno1
driver: r8169
version:
firmware-version:
expansion-rom-version:
bus-info: 0000:02:00.0
supports-statistics: yes
supports-test: no
supports-eeprom-access: no
supports-register-dump: yes
supports-priv-flags: no
lspci shows me:
02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 15)
Installing r8168-dkms fails with the following errors:
Setting up r8168-dkms (8.046.00-1) ...
Removing old r8168-8.046.00 DKMS files...
------------------------------
Deleting module version: 8.046.00
completely from the DKMS tree.
------------------------------
Done.
Loading new r8168-8.046.00 DKMS files...
Building for 5.2.0-050200rc6-generic 5.2.0-rc5-gpd-custom
Building initial module for 5.2.0-050200rc6-generic
Error! Bad return status for module build on kernel:
5.2.0-050200rc6-generic (x86_64)
Consult /var/lib/dkms/r8168/8.046.00/build/make.log for more information.
dpkg: error processing package r8168-dkms (--configure):
installed r8168-dkms package post-installation script subprocess
returned error exit status 10
Errors were encountered while processing:
r8168-dkms
E: Sub-process /usr/bin/dpkg returned an error code (1)
Does that help you?
Do you need more data?
Thank you very much in advance for hopefully looking into this matter.
Regards,
Karsten Wiborg
^ permalink raw reply
* Re: [net-next 08/15] iavf: Fix up debug print macro
From: Joe Perches @ 2019-06-29 20:42 UTC (permalink / raw)
To: Jeff Kirsher, davem; +Cc: netdev, nhorman, sassmann, Andrew Bowers
In-Reply-To: <20190628224932.3389-9-jeffrey.t.kirsher@intel.com>
On Fri, 2019-06-28 at 15:49 -0700, Jeff Kirsher wrote:
> This aligns the iavf_debug() macro with the other Intel drivers.
>
> Add the bus number, bus_id field to i40e_bus_info so output shows
> each physical port(i.e func) in following format:
> [[[[<domain>]:]<bus>]:][<slot>][.[<func>]]
> domains are numbered from 0 to ffff), bus (0-ff), slot (0-1f) and
> function (0-7).
>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf_osdep.h | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_osdep.h b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> index d39684558597..a452ce90679a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> @@ -44,8 +44,12 @@ struct iavf_virt_mem {
> #define iavf_allocate_virt_mem(h, m, s) iavf_allocate_virt_mem_d(h, m, s)
> #define iavf_free_virt_mem(h, m) iavf_free_virt_mem_d(h, m)
>
> -#define iavf_debug(h, m, s, ...) iavf_debug_d(h, m, s, ##__VA_ARGS__)
> -extern void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
> - __printf(3, 4);
> +#define iavf_debug(h, m, s, ...) \
> +do { \
> + if (((m) & (h)->debug_mask)) \
> + pr_info("iavf %02x:%02x.%x " s, \
> + (h)->bus.bus_id, (h)->bus.device, \
> + (h)->bus.func, ##__VA_ARGS__); \
> +} while (0)
Why not change the function to do this?
And if this is really wanted this particular way
the now unused function should be removed too.
But I suggest emitting at KERN_DEBUG and using
the more typical %pV vsprintf extension.
---
drivers/net/ethernet/intel/iavf/iavf_main.c | 25 ++++++++++++++-----------
drivers/net/ethernet/intel/iavf/iavf_osdep.h | 9 ++++++---
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 881561b36083..8504fd71d398 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -143,25 +143,28 @@ enum iavf_status iavf_free_virt_mem_d(struct iavf_hw *hw,
}
/**
- * iavf_debug_d - OS dependent version of debug printing
+ * _iavf_debug - OS dependent version of debug printing
* @hw: pointer to the HW structure
* @mask: debug level mask
- * @fmt_str: printf-type format description
+ * @fmt: printf-type format description
**/
-void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
+void _iavf_debug(const struct iavf_hw *hw, u32 mask, const char *fmt, ...)
{
- char buf[512];
- va_list argptr;
+ struct va_format vaf;
+ va_list args;
- if (!(mask & ((struct iavf_hw *)hw)->debug_mask))
+ if (!(hw->debug_mask & mask))
return;
- va_start(argptr, fmt_str);
- vsnprintf(buf, sizeof(buf), fmt_str, argptr);
- va_end(argptr);
+ va_start(args, fmt);
- /* the debug string is already formatted with a newline */
- pr_info("%s", buf);
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ pr_debug("iavf %02x:%02x.%x %pV",
+ hw->bus.bus_id, hw->bus.device, hw->bus.func, &vaf);
+
+ va_end(args);
}
/**
diff --git a/drivers/net/ethernet/intel/iavf/iavf_osdep.h b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
index d39684558597..0e6ac7d262c8 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
+++ b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
@@ -44,8 +44,11 @@ struct iavf_virt_mem {
#define iavf_allocate_virt_mem(h, m, s) iavf_allocate_virt_mem_d(h, m, s)
#define iavf_free_virt_mem(h, m) iavf_free_virt_mem_d(h, m)
-#define iavf_debug(h, m, s, ...) iavf_debug_d(h, m, s, ##__VA_ARGS__)
-extern void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
- __printf(3, 4);
+struct iavf_hw;
+
+__printf(3, 4)
+void _iavf_debug(const struct iavf_hw *hw, u32 mask, const char *fmt, ...);
+#define iavf_debug(hw, mask, fmt, ...) \
+ _iavf_debug(hw, mask, fmt, ##__VA_ARGS__)
#endif /* _IAVF_OSDEP_H_ */
^ permalink raw reply related
* Re: KASAN: use-after-free Write in xfrm_hash_rebuild
From: syzbot @ 2019-06-29 21:11 UTC (permalink / raw)
To: davem, fw, herbert, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs
In-Reply-To: <000000000000d028b30588fed102@google.com>
syzbot has bisected this bug to:
commit 1548bc4e0512700cf757192c106b3a20ab639223
Author: Florian Westphal <fw@strlen.de>
Date: Fri Jan 4 13:17:02 2019 +0000
xfrm: policy: delete inexact policies from inexact list on hash rebuild
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1734cba9a00000
start commit: 249155c2 Merge branch 'parisc-5.2-4' of git://git.kernel.o..
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=14b4cba9a00000
console output: https://syzkaller.appspot.com/x/log.txt?x=10b4cba9a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=9a31528e58cc12e2
dashboard link: https://syzkaller.appspot.com/bug?extid=0165480d4ef07360eeda
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16cf37c3a00000
Reported-by: syzbot+0165480d4ef07360eeda@syzkaller.appspotmail.com
Fixes: 1548bc4e0512 ("xfrm: policy: delete inexact policies from inexact
list on hash rebuild")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: r8169 not working on 5.2.0rc6 with GPD MicroPC
From: Karsten Wiborg @ 2019-06-29 21:19 UTC (permalink / raw)
To: nic_swsd, romieu; +Cc: netdev
In-Reply-To: <0a560a5e-17f2-f6ff-1dad-66907133e9c2@web.de>
Hi,
short update with some more data:
# more /var/lib/dkms/r8168/8.046.00/build/make.log
DKMS make.log for r8168-8.046.00 for kernel 5.2.0-050200rc6-generic (x86_64)
Sat 29 Jun 2019 10:11:15 PM CEST
make: Entering directory '/usr/src/linux-headers-5.2.0-050200rc6-generic'
CC [M] /var/lib/dkms/r8168/8.046.00/build/r8168_n.o
CC [M] /var/lib/dkms/r8168/8.046.00/build/r8168_asf.o
CC [M] /var/lib/dkms/r8168/8.046.00/build/rtl_eeprom.o
CC [M] /var/lib/dkms/r8168/8.046.00/build/rtltool.o
/var/lib/dkms/r8168/8.046.00/build/r8168_n.c: In function ‘rtl8168_down’:
/var/lib/dkms/r8168/8.046.00/build/r8168_n.c:28376:9: error: implicit
declaration of function ‘synch
ronize_sched’; did you mean ‘synchronize_net’?
[-Werror=implicit-function-declaration]
synchronize_sched(); /* FIXME: should this be
synchronize_irq()? */
^~~~~~~~~~~~~~~~~
synchronize_net
cc1: some warnings being treated as errors
make[1]: *** [scripts/Makefile.build:278:
/var/lib/dkms/r8168/8.046.00/build/r8168_n.o] Error 1
make: *** [Makefile:1595: _module_/var/lib/dkms/r8168/8.046.00/build]
Error 2
make: Leaving directory '/usr/src/linux-headers-5.2.0-050200rc6-generic'
# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 8.3.0-6'
--with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++
--prefix=/usr --with-gcc-major-version-only --program-suffix=-8
--program-prefix=x86_64-linux-gnu- --enable-shared
--enable-linker-build-id --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix --libdir=/usr/lib
--enable-nls --enable-bootstrap --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-vtable-verify --enable-libmpx --enable-plugin
--enable-default-pie --with-system-zlib --with-target-system-zlib
--enable-objc-gc=auto --enable-multiarch --disable-werror
--with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32
--enable-multilib --with-tune=generic
--enable-offload-targets=nvptx-none --without-cuda-driver
--enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 8.3.0 (Debian 8.3.0-6)
Regards,
Karsten Wiborg
On 6/29/19 10:34 PM, Karsten Wiborg wrote:
> Hello,
>
> writing to you because of the r8168-dkms-README.Debian.
>
> I am using a GPD MicroPC running Debian Buster with Kernel:
> Linux praktifix 5.2.0-050200rc6-generic #201906222033 SMP Sun Jun 23
> 00:36:46 UTC 2019 x86_64 GNU/Linux
>
>
> Got the Kernel from:
> https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.2-rc6/
> Reason for the Kernel: it includes Hans DeGoedes necessary fixes for the
> GPD MicroPC, see:
> https://github.com/jwrdegoede/linux-sunxi
> (btw. I also tried Hans' 5.2.0rc5-kernel which also did not work with
> respect to Ethernet). Googling of course didn't help out either.
>
>
> My GPD MicroPC with running r8169 gives the following lines in dmesg:
> ...
> [ 2.839485] libphy: r8169: probed
> [ 2.839776] r8169 0000:02:00.0 eth0: RTL8168h/8111h,
> 00:00:00:00:00:00, XID 541, IRQ 126
> [ 2.839779] r8169 0000:02:00.0 eth0: jumbo features [frames: 9200
> bytes, tx checksumming: ko]
> ...
> [ 2.897924] r8169 0000:02:00.0 eno1: renamed from eth0
>
>
> ip addr show gives me:
> # ip addr show
> ...
> 2: eno1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group
> default qlen 1000
> link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> ...
>
>
> ethtool gives me:
> # ethtool -i eno1
> driver: r8169
> version:
> firmware-version:
> expansion-rom-version:
> bus-info: 0000:02:00.0
> supports-statistics: yes
> supports-test: no
> supports-eeprom-access: no
> supports-register-dump: yes
> supports-priv-flags: no
>
>
> lspci shows me:
> 02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 15)
>
>
> Installing r8168-dkms fails with the following errors:
> Setting up r8168-dkms (8.046.00-1) ...
> Removing old r8168-8.046.00 DKMS files...
>
> ------------------------------
> Deleting module version: 8.046.00
> completely from the DKMS tree.
> ------------------------------
> Done.
> Loading new r8168-8.046.00 DKMS files...
> Building for 5.2.0-050200rc6-generic 5.2.0-rc5-gpd-custom
> Building initial module for 5.2.0-050200rc6-generic
> Error! Bad return status for module build on kernel:
> 5.2.0-050200rc6-generic (x86_64)
> Consult /var/lib/dkms/r8168/8.046.00/build/make.log for more information.
> dpkg: error processing package r8168-dkms (--configure):
> installed r8168-dkms package post-installation script subprocess
> returned error exit status 10
> Errors were encountered while processing:
> r8168-dkms
> E: Sub-process /usr/bin/dpkg returned an error code (1)
>
>
> Does that help you?
> Do you need more data?
> Thank you very much in advance for hopefully looking into this matter.
>
> Regards,
> Karsten Wiborg
>
^ permalink raw reply
* Re: [RFC iproute2 1/1] ip: netns: add mounted state file for each netns
From: Matteo Croce @ 2019-06-29 21:45 UTC (permalink / raw)
To: David Howells
Cc: Nicolas Dichtel, Alexander Aring, netdev, linux-fsdevel, kernel
In-Reply-To: <CAGnkfhwe6p412q4sATwX=3ht4+NxKJDOFihRG=iwvXqWUtwgLQ@mail.gmail.com>
On Fri, Jun 28, 2019 at 7:06 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Fri, Jun 28, 2019 at 6:27 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> >
> > > David Howells was working on a mount notification mechanism:
> > > https://lwn.net/Articles/760714/
> > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications
> > >
> > > I don't know what is the status of this series.
> >
> > It's still alive. I just posted a new version on it. I'm hoping, possibly
> > futiley, to get it in in this merge window.
> >
> > David
>
> Hi all,
>
> this could cause a clash if I create a netns with name ending with .mounted.
>
> $ sudo ip/ip netns add ns1.mounted
> $ sudo ip/ip netns add ns1
> Cannot create namespace file "/var/run/netns/ns1.mounted": File exists
> Cannot remove namespace file "/var/run/netns/ns1.mounted": Device or
> resource busy
>
> If you want to go along this road, please either:
> - disallow netns creation with names ending with .mounted
> - or properly document it in the manpage
>
> Regards,
> --
> Matteo Croce
> per aspera ad upstream
BTW, this breaks the namespace listing:
# ip netns add test
# ip netns list
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test.mounted
test
A better choice IMHO could be to create a temporary file before the
placeholder, and delete it after the bind mount, so an inotify watcher
can listen for the delete event.
For example, when creating the namespace "foo":
- create /var/run/netns/.foo.mounting
- create /var/run/netns/foo
- bind mount from /proc/.. to /var/run/netns/foo
- remove /var/run/netns/.foo.mounting
and exclude .*.mounting from the netns listing
Or, announce netns creation/deletion in some other way (dbus?).
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* loss of connectivity after enabling vlan_filtering
From: vtolkm @ 2019-06-29 22:01 UTC (permalink / raw)
To: netdev
* DSA MV88E6060
* iproute2 v.5.0.0-2.0
* OpenWRT 19.07 with kernel 4.14.131 armv7l
_______
after
# bridge v a dev {bridge} self vid {n} untagged pvid
or with the device enslaved in the br
# bridge v a dev {device} vid {n} untagged pvid
I am hitting a roadblock when executing
# sysctl -w /sys/class/net/{bridge}/bridge/vlan_filtering=1
there is immediate loss of connectivity with the node until
# sysctl -w /sys/class/net/{bridge}/bridge/vlan_filtering=0
Since writing here I apparently trust that above is unexpected and I
cannot figure out what is (going) wrong.
Tried some variation (enabling VID on the client when appropriate) but
having met the same outcome.
# bridge v a dev {bridge} self vid {n} untagged
# bridge v a dev {bridge} self vid {n} pvid
# bridge v a dev {bridge} self vid {n}
# bridge v s
reflects any such change.
# ip -d l sh type bridge
br-mgt: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state
UP mode DEFAULT group default qlen 1000
link/ether 1e:8e:c2:3c:b8:35 brd ff:ff:ff:ff:ff:ff promiscuity 0
bridge forward_delay 200 hello_time 200 max_age 2000 ageing_time
30000 stp_state 0 priority 32767 vlan_filtering 0 vlan_protocol 802.1Q
bridge_id 7fff.1E:8E:C2:3C:B8:35 designated_root 7fff.1E:8E:C2:3C:B8:35
root_port 0 root_path_cost 0 topology_change 0 topology_change_detected
0 hello_timer 0.00 tcn_timer 0.00 topology_change_timer 0.00
gc_timer 168.58 vlan_default_pvid 1 vlan_stats_enabled 0 group_fwd_mask
0 group_address 01:80:c2:00:00:00 mcast_snooping 0 mcast_router 1
mcast_query_use_ifaddr 0 mcast_querier 0 mcast_hash_elasticity 4
mcast_hash_max 512 mcast_last_member_count 2 mcast_startup_query_count 2
mcast_last_member_interval 100 mcast_membership_interval 26000
mcast_querier_interval 25500 mcast_query_interval 12500
mcast_query_response_interval 1000 mcast_startup_query_interval 3125
mcast_stats_enabled 0 mcast_igmp_version 2 mcast_mld_version 1
nf_call_iptables 0 nf_call_ip6tables 0 nf_call_arptables 0 addrgenmode
stable_secret numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535
______________________
* kernel conf
---
CONFIG_NETFILTER=y
CONFIG_NETFILTER_ADVANCED=y
CONFIG_BRIDGE_NETFILTER=m
CONFIG_NETFILTER_INGRESS=y
CONFIG_NETFILTER_NETLINK=m
CONFIG_NETFILTER_FAMILY_BRIDGE=y
CONFIG_NETFILTER_FAMILY_ARP=y
# CONFIG_NETFILTER_NETLINK_ACCT is not set
CONFIG_NETFILTER_NETLINK_QUEUE=m
CONFIG_NETFILTER_NETLINK_LOG=m
# CONFIG_NETFILTER_NETLINK_GLUE_CT is not set
CONFIG_IP_NF_MATCH_RPFILTER=m
CONFIG_IP_NF_FILTER=m
CONFIG_IP_NF_ARPFILTER=m
CONFIG_IP6_NF_MATCH_RPFILTER=m
CONFIG_IP6_NF_FILTER=m
CONFIG_BRIDGE_EBT_T_FILTER=m
CONFIG_ATM_BR2684_IPFILTER=y
CONFIG_BRIDGE_VLAN_FILTERING=y
CONFIG_HAVE_NET_DSA=y
CONFIG_NET_DSA=y
CONFIG_NET_DSA_TAG_DSA=y
CONFIG_NET_DSA_TAG_EDSA=y
CONFIG_NET_DSA_TAG_TRAILER=y
CONFIG_BT_BNEP_MC_FILTER=y
CONFIG_BT_BNEP_PROTO_FILTER=y
# CONFIG_NET_DSA_BCM_SF2 is not set
# CONFIG_NET_DSA_LOOP is not set
# CONFIG_NET_DSA_MT7530 is not set
CONFIG_NET_DSA_MV88E6060=y
CONFIG_NET_DSA_MV88E6XXX=y
CONFIG_NET_DSA_MV88E6XXX_GLOBAL2=y
# CONFIG_NET_DSA_QCA8K is not set
# CONFIG_NET_DSA_SMSC_LAN9303_I2C is not set
# CONFIG_NET_DSA_SMSC_LAN9303_MDIO is not set
# CONFIG_HNS_DSAF is not set
CONFIG_PPP_FILTER=y
CONFIG_IPPP_FILTER=y
^ permalink raw reply
* Re: r8169 not working on 5.2.0rc6 with GPD MicroPC
From: Heiner Kallweit @ 2019-06-29 22:09 UTC (permalink / raw)
To: Karsten Wiborg, nic_swsd, romieu; +Cc: netdev
In-Reply-To: <0a560a5e-17f2-f6ff-1dad-66907133e9c2@web.de>
On 29.06.2019 22:34, Karsten Wiborg wrote:
> Hello,
>
> writing to you because of the r8168-dkms-README.Debian.
>
> I am using a GPD MicroPC running Debian Buster with Kernel:
> Linux praktifix 5.2.0-050200rc6-generic #201906222033 SMP Sun Jun 23
> 00:36:46 UTC 2019 x86_64 GNU/Linux
>
>
> Got the Kernel from:
> https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.2-rc6/
> Reason for the Kernel: it includes Hans DeGoedes necessary fixes for the
> GPD MicroPC, see:
> https://github.com/jwrdegoede/linux-sunxi
> (btw. I also tried Hans' 5.2.0rc5-kernel which also did not work with
> respect to Ethernet). Googling of course didn't help out either.
>
>
> My GPD MicroPC with running r8169 gives the following lines in dmesg:
> ...
If r8169 (the mainline driver) is running, why do you want to switch
to r8168 (the Realtek vendor driver)? The latter is not supported by
the kernel community.
> [ 2.839485] libphy: r8169: probed
> [ 2.839776] r8169 0000:02:00.0 eth0: RTL8168h/8111h,
> 00:00:00:00:00:00, XID 541, IRQ 126
> [ 2.839779] r8169 0000:02:00.0 eth0: jumbo features [frames: 9200
> bytes, tx checksumming: ko]
> ...
> [ 2.897924] r8169 0000:02:00.0 eno1: renamed from eth0
>
>
> ip addr show gives me:
> # ip addr show
> ...
> 2: eno1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group
> default qlen 1000
> link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> ...
>
Seems like the network isn't started.
>
> ethtool gives me:
> # ethtool -i eno1
> driver: r8169
> version:
> firmware-version:
> expansion-rom-version:
> bus-info: 0000:02:00.0
> supports-statistics: yes
> supports-test: no
> supports-eeprom-access: no
> supports-register-dump: yes
> supports-priv-flags: no
>
>
> lspci shows me:
> 02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 15)
>
>
> Installing r8168-dkms fails with the following errors:
> Setting up r8168-dkms (8.046.00-1) ...
> Removing old r8168-8.046.00 DKMS files...
>
> ------------------------------
> Deleting module version: 8.046.00
> completely from the DKMS tree.
> ------------------------------
> Done.
> Loading new r8168-8.046.00 DKMS files...
> Building for 5.2.0-050200rc6-generic 5.2.0-rc5-gpd-custom
> Building initial module for 5.2.0-050200rc6-generic
> Error! Bad return status for module build on kernel:
> 5.2.0-050200rc6-generic (x86_64)
> Consult /var/lib/dkms/r8168/8.046.00/build/make.log for more information.
> dpkg: error processing package r8168-dkms (--configure):
> installed r8168-dkms package post-installation script subprocess
> returned error exit status 10
> Errors were encountered while processing:
> r8168-dkms
> E: Sub-process /usr/bin/dpkg returned an error code (1)
>
>
> Does that help you?
> Do you need more data?
> Thank you very much in advance for hopefully looking into this matter.
>
> Regards,
> Karsten Wiborg
> .
>
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: Andrew Lunn @ 2019-06-29 22:49 UTC (permalink / raw)
To: vtolkm; +Cc: netdev
In-Reply-To: <e5252bf0-f9c1-3e40-aebd-8c091dbb3e64@gmail.com>
On Sun, Jun 30, 2019 at 12:01:38AM +0200, vtolkm@googlemail.com wrote:
> * DSA MV88E6060
> * iproute2 v.5.0.0-2.0
> * OpenWRT 19.07 with kernel 4.14.131 armv7l
The mv88e6060 driver is very simple. It has no support for VLANs. It
does not even have support for offloading bridging between ports to
the switch.
The data sheet for this device is open. So if you want to hack on the
driver, you could do.
Andrew
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: vtolkm @ 2019-06-29 23:04 UTC (permalink / raw)
To: netdev; +Cc: Andrew Lunn
In-Reply-To: <20190629224927.GA26554@lunn.ch>
On 30/06/2019 00:49, Andrew Lunn wrote:
> On Sun, Jun 30, 2019 at 12:01:38AM +0200, vtolkm@googlemail.com wrote:
>> * DSA MV88E6060
>> * iproute2 v.5.0.0-2.0
>> * OpenWRT 19.07 with kernel 4.14.131 armv7l
> The mv88e6060 driver is very simple. It has no support for VLANs. It
> does not even have support for offloading bridging between ports to
> the switch.
>
> The data sheet for this device is open. So if you want to hack on the
> driver, you could do.
>
> Andrew
Are you sure? That is a bit confusing after reading
https://lore.kernel.org/patchwork/patch/575746/
^ permalink raw reply
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Andrew Lunn @ 2019-06-29 23:06 UTC (permalink / raw)
To: Ido Schimmel
Cc: f.fainelli, vivien.didelot, netdev@vger.kernel.org, Jiri Pirko,
linux@armlinux.org.uk, davem@davemloft.net
In-Reply-To: <20190629153135.GA17143@splinter>
> We have some tests under tools/testing/selftests/net/forwarding/, but
> not so much for MC. However, even if such tests were to be contributed,
> would you be able to run them on your hardware? I remember that in the
> past you complained about the number of required ports.
Hi Ido
Most devices have 4 or 5 ports. Some have more, some less. Given that
the current tests need a second port for every port under test, this
limits you to 2 ports under test. For IGMP snooping and MC in general,
i don't think that is enough. You need one port as the source of the
MC traffic, one port with a host interested in the traffic, and one
port without interest in the traffic. You can then test that traffic
is correctly forwarded and blocked.
The testing we do with DSA makes use of a test host with 4 interfaces,
and connect them one to one to the switch. That gives us 4 ports under
test, and so that gives us many more possibilities for running
interesting tests.
Andrew
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: Andrew Lunn @ 2019-06-29 23:11 UTC (permalink / raw)
To: vtolkm; +Cc: netdev
In-Reply-To: <6226b473-b232-e1d3-40e9-18d118dd82c4@gmail.com>
On Sun, Jun 30, 2019 at 01:04:50AM +0200, vtolkm@googlemail.com wrote:
>
> On 30/06/2019 00:49, Andrew Lunn wrote:
> > On Sun, Jun 30, 2019 at 12:01:38AM +0200, vtolkm@googlemail.com wrote:
> >> * DSA MV88E6060
> >> * iproute2 v.5.0.0-2.0
> >> * OpenWRT 19.07 with kernel 4.14.131 armv7l
> > The mv88e6060 driver is very simple. It has no support for VLANs. It
> > does not even have support for offloading bridging between ports to
> > the switch.
> >
> > The data sheet for this device is open. So if you want to hack on the
> > driver, you could do.
> >
> > Andrew
>
> Are you sure? That is a bit confusing after reading
> https://lore.kernel.org/patchwork/patch/575746/
Quoting that patch:
This commit implements the switchdev operations to add, delete
and dump VLANs for the Marvell 88E6352 and compatible switch
chips.
Vivien added support for the 6352. That uses the mv88e6xxx driver, not
the mv88e6060. And by compatible switches, he meant those in the 6352
family, so 6172 6176 6240 6352 and probably the 6171 6175 6350 6351.
Andrew
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: vtolkm @ 2019-06-29 23:23 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190629231119.GC26554@lunn.ch>
On 30/06/2019 01:11, Andrew Lunn wrote:
> On Sun, Jun 30, 2019 at 01:04:50AM +0200, vtolkm@googlemail.com wrote:
>> On 30/06/2019 00:49, Andrew Lunn wrote:
>>> On Sun, Jun 30, 2019 at 12:01:38AM +0200, vtolkm@googlemail.com wrote:
>>>> * DSA MV88E6060
>>>> * iproute2 v.5.0.0-2.0
>>>> * OpenWRT 19.07 with kernel 4.14.131 armv7l
>>> The mv88e6060 driver is very simple. It has no support for VLANs. It
>>> does not even have support for offloading bridging between ports to
>>> the switch.
>>>
>>> The data sheet for this device is open. So if you want to hack on the
>>> driver, you could do.
>>>
>>> Andrew
>> Are you sure? That is a bit confusing after reading
>> https://lore.kernel.org/patchwork/patch/575746/
> Quoting that patch:
>
> This commit implements the switchdev operations to add, delete
> and dump VLANs for the Marvell 88E6352 and compatible switch
> chips.
>
> Vivien added support for the 6352. That uses the mv88e6xxx driver, not
> the mv88e6060. And by compatible switches, he meant those in the 6352
> family, so 6172 6176 6240 6352 and probably the 6171 6175 6350 6351.
>
> Andrew
A simple soul might infer that mv88e6xxx includes MV88E6060, at least
that happened to me apparently (being said simpleton).
That may as it be, and pardon my continued ignorance, how is it
explained then that a command as
# bridge v a dev {bridge} self vid {n} untagged pvid
reflects in
# bridge v s
a/o
# bridge mo
?
If the commands are not implemented one would expect them to fail in the
first place or not reflecting a change at all?
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Andy Lutomirski @ 2019-06-29 23:47 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andy Lutomirski, Stephen Smalley, James Morris, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Network Development, Chun-Yi Lee, Daniel Borkmann, LSM List
In-Reply-To: <CACdnJuuy7-tkj86njAqtdJ3dUMu-2T8a2y8DC3fMKBK0z9J6ag@mail.gmail.com>
On Fri, Jun 28, 2019 at 11:47 AM Matthew Garrett <mjg59@google.com> wrote:
>
> On Thu, Jun 27, 2019 at 4:27 PM Andy Lutomirski <luto@kernel.org> wrote:
> > They're really quite similar in my mind. Certainly some things in the
> > "integrity" category give absolutely trivial control over the kernel
> > (e.g. modules) while others make it quite challenging (ioperm), but
> > the end result is very similar. And quite a few "confidentiality"
> > things genuinely do allow all kernel memory to be read.
> >
> > I agree that finer-grained distinctions could be useful. My concern is
> > that it's a tradeoff, and the other end of the tradeoff is an ABI
> > stability issue. If someone decides down the road that some feature
> > that is currently "integrity" can be split into a narrow "integrity"
> > feature and a "confidentiality" feature then, if the user policy knows
> > about the individual features, there's a risk of breaking people's
> > systems. If we keep the fine-grained control, do we have a clear
> > compatibility story?
>
> My preference right now is to retain the fine-grained aspect of things
> in the internal API, simply because it'll be more annoying to add it
> back later if we want to. I don't want to expose it via the Lockdown
> user facing API for the reasons you've described, but it's not
> impossible that another LSM would find a way to do this reasonably.
> Does it seem reasonable to punt this discussion out to the point where
> another LSM tries to do something with this information, based on the
> implementation they're attempting?
I think I can get behind this, as long as it's clear to LSM authors
that this list is only a little bit stable. I can certainly see the
use for the fine-grained info being available for auditing.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Andy Lutomirski @ 2019-06-30 0:12 UTC (permalink / raw)
To: Song Liu, linux-security
Cc: Andy Lutomirski, Networking, bpf, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Lorenz Bauer, Jann Horn, Greg KH,
linux-abi@vger.kernel.org, kees@chromium.org
In-Reply-To: <3C595328-3ABE-4421-9772-8D41094A4F57@fb.com>
On Fri, Jun 28, 2019 at 12:05 PM Song Liu <songliubraving@fb.com> wrote:
>
> Hi Andy,
>
> > On Jun 27, 2019, at 4:40 PM, Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On 6/27/19 1:19 PM, Song Liu wrote:
> >> This patch introduce unprivileged BPF access. The access control is
> >> achieved via device /dev/bpf. Users with write access to /dev/bpf are able
> >> to call sys_bpf().
> >> Two ioctl command are added to /dev/bpf:
> >> The two commands enable/disable permission to call sys_bpf() for current
> >> task. This permission is noted by bpf_permitted in task_struct. This
> >> permission is inherited during clone(CLONE_THREAD).
> >> Helper function bpf_capable() is added to check whether the task has got
> >> permission via /dev/bpf.
> >
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 0e079b2298f8..79dc4d641cf3 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -9134,7 +9134,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
> >> env->insn_aux_data[i].orig_idx = i;
> >> env->prog = *prog;
> >> env->ops = bpf_verifier_ops[env->prog->type];
> >> - is_priv = capable(CAP_SYS_ADMIN);
> >> + is_priv = bpf_capable(CAP_SYS_ADMIN);
> >
> > Huh? This isn't a hardening measure -- the "is_priv" verifier mode allows straight-up leaks of private kernel state to user mode.
> >
> > (For that matter, the pending lockdown stuff should possibly consider this a "confidentiality" issue.)
> >
> >
> > I have a bigger issue with this patch, though: it's a really awkward way to pretend to have capabilities. For bpf, it seems like you could make this be a *real* capability without too much pain since there's only one syscall there. Just find a way to pass an fd to /dev/bpf into the syscall. If this means you need a new bpf_with_cap() syscall that takes an extra argument, so be it. The old bpf() syscall can just translate to bpf_with_cap(..., -1).
> >
> > For a while, I've considered a scheme I call "implicit rights". There would be a directory in /dev called /dev/implicit_rights. This would either be part of devtmpfs or a whole new filesystem -- it would *not* be any other filesystem. The contents would be files that can't be read or written and exist only in memory. You create them with a privileged syscall. Certain actions that are sensitive but not at the level of CAP_SYS_ADMIN (use of large-attack-surface bpf stuff, creation of user namespaces, profiling the kernel, etc) could require an "implicit right". When you do them, if you don't have CAP_SYS_ADMIN, the kernel would do a path walk for, say, /dev/implicit_rights/bpf and, if the object exists, can be opened, and actually refers to the "bpf" rights object, then the action is allowed. Otherwise it's denied.
> >
> > This is extensible, and it doesn't require the rather ugly per-task state of whether it's enabled.
> >
> > For things like creation of user namespaces, there's an existing API, and the default is that it works without privilege. Switching it to an implicit right has the benefit of not requiring code changes to programs that already work as non-root.
> >
> > But, for BPF in particular, this type of compatibility issue doesn't exist now. You already can't use most eBPF functionality without privilege. New bpf-using programs meant to run without privilege are *new*, so they can use a new improved API. So, rather than adding this obnoxious ioctl, just make the API explicit, please.
> >
> > Also, please cc: linux-abi next time.
>
> Thanks for your inputs.
>
> I think we need to clarify the use case here. In this case, we are NOT
> thinking about creating new tools for unprivileged users. Instead, we
> would like to use existing tools without root.
I read patch 4, and I interpret it very differently. Patches 2-4 are
creating a new version of libbpf and a new version of bpftool. Given
this, I see no real justification for adding a new in-kernel per-task
state instead of just pushing the complexity into libbpf.
> On the kernel side, we are not planning provides a subset of safe
> features for unprivileged users. The permission here is all-or-nothing.
This may be a showstopper. I think this series needs an extremely
clear explanation of the security implications of providing access to
/dev/bpf. Is it just exposing more attack surface for kernel bugs, or
is it, *by design*, exposing new privileges. Given the is_priv change
that I pointed out upthread, it appears to be the latter, and I'm
wondering how this is a reasonable thing to do.
>
> Introducing bpf_with_cap() syscall means we need teach these tools to
> manage the fd, and use the new API when necessary. This is clearly not
> easy.
How hard can it be? I looked the the libbpf sources, and there are
really very few call sites of sys_bpf(). You could update all of them
with a very small patch.
Also, on a quick survey of kernel/bpf's capable() calls, I feel like
this series may be misguided. If you want to enable unprivileged or
less privileged use of bpf(), how about going through all of the
capable() calls and handling them one-by-one as appropriate. Here's a
random survey:
map_freeze(): It looks like the only reason for a capable() call is
that there isn't a clear permission model right now defining who owns
a map and therefore may freeze it. Could you not use a check along
the lines of capable_wrt_inode_uidgid() instead of capable()?
if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&
(attr->prog_flags & BPF_F_ANY_ALIGNMENT) &&
!capable(CAP_SYS_ADMIN))
return -EPERM;
I'm not sure why this is needed at all? Is it a DoS mitigation? If
so, couldn't you find a way to acocunt for inefficient unaligned
access similarly to how you account for complexity in general?
if (attr->insn_cnt == 0 ||
attr->insn_cnt > (capable(CAP_SYS_ADMIN) ?
BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
return -E2BIG;
This is similar. I could imagine a cgroup setting that limits bpf
program complexity. This is very, very different type of privilege
than reading kernel memory.
if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
type != BPF_PROG_TYPE_CGROUP_SKB &&
!capable(CAP_SYS_ADMIN))
return -EPERM;
I suspect you could just delete this check or expand the allowable
unprivileged program types after auditing that the other types have
appropriately restrictive verifiers and require appropriate
permissions to *run* the programs.
In bpf_prog_attach():
if (!capable(CAP_NET_ADMIN))
return -EPERM;
This looks like it wants to be ns_capable() after you audit the code
to make sure it's safe enough.
bpf_prog_get_fd_by_id(): I really think you just need a real
permission model. Anyone can create a file on a filesystem, and there
are reasonable policies that allow appropriate users to open existing
files by name without CAP_SYS_ADMIN. Similarly, anyone can create a
key in the kernel keyring subsystem, and there are well-defined rules
under which someone can access an existing key by name. I think you
should come up with a way to handle this for bpf. Adding a bpffs for
this seems like a decent approach.
bpf_prog_get_info_by_id():
if (!capable(CAP_SYS_ADMIN)) {
info.jited_prog_len = 0;
info.xlated_prog_len = 0;
info.nr_jited_ksyms = 0;
info.nr_jited_func_lens = 0;
info.nr_func_info = 0;
info.nr_line_info = 0;
info.nr_jited_line_info = 0;
goto done;
}
It looks like someone decided this information was sensitive if you
query someone else's program. Again, there are sensible ways to
address this.
Finally, in the verifier:
is_priv = capable(CAP_SYS_ADMIN);
That should just be left alone. Arguably you could make a new
capability CAP_BPF_LEAK_KERNEL_ADDRESSES or similar for this.
As it stands, it seems like bpf() was designed under the assumption
that the kind of security model needed to make it work well for
unprivileged or differently-privileged users would be developed later.
But, instead of developing such a security model, this patch is
introducing a whole new Linux "capability" that just disables all
security. From my perspective, this seems like a bad idea.
So, if anyone cares about my opinion, NAK to the whole concept.
Please instead work on fixing this for real, one capable() check at a
time.
^ permalink raw reply
* Re: r8169 not working on 5.2.0rc6 with GPD MicroPC
From: Karsten Wiborg @ 2019-06-30 0:14 UTC (permalink / raw)
To: Heiner Kallweit, nic_swsd, romieu; +Cc: netdev
In-Reply-To: <85548ec0-350b-118f-a60c-4be2235d5e4e@gmail.com>
Hi Heiner,
thanks for the speedy reply.
On 6/30/19 12:09 AM, Heiner Kallweit wrote:
> If r8169 (the mainline driver) is running, why do you want to switch
> to r8168 (the Realtek vendor driver)? The latter is not supported by
> the kernel community.
Well I did install r8168 because r8169 is not working.
Didn't even get to see the MAC of the NIC.
>> 2: eno1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group
>> default qlen 1000
>> link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> Seems like the network isn't started.
Jepp, that is the output from the r8169.
Regards,
Karsten
^ permalink raw reply
* Re: [PATCH rdma-next v4 06/17] RDMA/counter: Add "auto" configuration mode support
From: Jason Gunthorpe @ 2019-06-30 0:32 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190618172625.13432-7-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:26:14PM +0300, Leon Romanovsky wrote:
> +/**
> + * rdma_counter_bind_qp_auto - Check and bind the QP to a counter base on
> + * the auto-mode rule
> + */
> +int rdma_counter_bind_qp_auto(struct ib_qp *qp, u8 port)
> +{
> + struct rdma_port_counter *port_counter;
> + struct ib_device *dev = qp->device;
> + struct rdma_counter *counter;
> + int ret;
> +
> + if (!rdma_is_port_valid(dev, port))
> + return -EINVAL;
> +
> + port_counter = &dev->port_data[port].port_counter;
> + if (port_counter->mode.mode != RDMA_COUNTER_MODE_AUTO)
> + return 0;
> +
> + counter = rdma_get_counter_auto_mode(qp, port);
> + if (counter) {
> + ret = __rdma_counter_bind_qp(counter, qp);
> + if (ret) {
> + rdma_restrack_put(&counter->res);
> + return ret;
> + }
> + kref_get(&counter->kref);
The counter is left in the xarray while the kref is zero, this
kref_get is wrong..
Using two kref like things at the same time is a bad idea, the
'rdma_get_counter_auto_mode' should return the kref held, not the
restrack get. The restrack_del doesn't happen as long as the kref is
positive, so we don't need the retrack thing here..
> + } else {
> + counter = rdma_counter_alloc(dev, port, RDMA_COUNTER_MODE_AUTO);
> + if (!counter)
> + return -ENOMEM;
> +
> + auto_mode_init_counter(counter, qp, port_counter->mode.mask);
> +
> + ret = __rdma_counter_bind_qp(counter, qp);
> + if (ret)
> + goto err_bind;
> +
> + rdma_counter_res_add(counter, qp);
> + if (!rdma_restrack_get(&counter->res)) {
> + ret = -EINVAL;
> + goto err_get;
> + }
and this shouldn't be needed as the kref is inited to 1 by the
rdma_counter_alloc..
> + }
> +
> + return 0;
> +
> +err_get:
> + __rdma_counter_unbind_qp(qp);
> + __rdma_counter_dealloc(counter);
> +err_bind:
> + rdma_counter_free(counter);
> + return ret;
> +}
And then all this error unwind and all the twisty __ functions should
just be a single kref_put and the release should handle everything.
Jason
^ permalink raw reply
* Re: [PATCH net 1/1] net: openvswitch: fix csum updates for MPLS actions
From: Pravin Shelar @ 2019-06-30 0:33 UTC (permalink / raw)
To: John Hurley
Cc: Linux Kernel Network Developers, David S. Miller, Simon Horman,
Jakub Kicinski, oss-drivers
In-Reply-To: <1561642650-1974-1-git-send-email-john.hurley@netronome.com>
On Thu, Jun 27, 2019 at 6:37 AM John Hurley <john.hurley@netronome.com> wrote:
>
> Skbs may have their checksum value populated by HW. If this is a checksum
> calculated over the entire packet then the CHECKSUM_COMPLETE field is
> marked. Changes to the data pointer on the skb throughout the network
> stack still try to maintain this complete csum value if it is required
> through functions such as skb_postpush_rcsum.
>
> The MPLS actions in Open vSwitch modify a CHECKSUM_COMPLETE value when
> changes are made to packet data without a push or a pull. This occurs when
> the ethertype of the MAC header is changed or when MPLS lse fields are
> modified.
>
> The modification is carried out using the csum_partial function to get the
> csum of a buffer and add it into the larger checksum. The buffer is an
> inversion of the data to be removed followed by the new data. Because the
> csum is calculated over 16 bits and these values align with 16 bits, the
> effect is the removal of the old value from the CHECKSUM_COMPLETE and
> addition of the new value.
>
> However, the csum fed into the function and the outcome of the
> calculation are also inverted. This would only make sense if it was the
> new value rather than the old that was inverted in the input buffer.
>
> Fix the issue by removing the bit inverts in the csum_partial calculation.
>
> The bug was verified and the fix tested by comparing the folded value of
> the updated CHECKSUM_COMPLETE value with the folded value of a full
> software checksum calculation (reset skb->csum to 0 and run
> skb_checksum_complete(skb)). Prior to the fix the outcomes differed but
> after they produce the same result.
>
> Fixes: 25cd9ba0abc0 ("openvswitch: Add basic MPLS support to kernel")
> Fixes: bc7cc5999fd3 ("openvswitch: update checksum in {push,pop}_mpls")
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
Thanks for fixing it.
Acked-by: Pravin B Shelar <pshelar@ovn.org>
> net/openvswitch/actions.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 151518d..bd13146 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -166,8 +166,7 @@ static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
> if (skb->ip_summed == CHECKSUM_COMPLETE) {
> __be16 diff[] = { ~(hdr->h_proto), ethertype };
>
> - skb->csum = ~csum_partial((char *)diff, sizeof(diff),
> - ~skb->csum);
> + skb->csum = csum_partial((char *)diff, sizeof(diff), skb->csum);
> }
>
> hdr->h_proto = ethertype;
> @@ -259,8 +258,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
> if (skb->ip_summed == CHECKSUM_COMPLETE) {
> __be32 diff[] = { ~(stack->label_stack_entry), lse };
>
> - skb->csum = ~csum_partial((char *)diff, sizeof(diff),
> - ~skb->csum);
> + skb->csum = csum_partial((char *)diff, sizeof(diff), skb->csum);
> }
>
> stack->label_stack_entry = lse;
> --
> 2.7.4
>
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: vtolkm @ 2019-06-30 0:37 UTC (permalink / raw)
To: netdev; +Cc: Andrew Lunn
In-Reply-To: <53bd8ffc-1c0a-334d-67d5-3a74b76670e8@gmail.com>
On 30/06/2019 01:23, vtolkm@gmail.com wrote:
> On 30/06/2019 01:11, Andrew Lunn wrote:
>> On Sun, Jun 30, 2019 at 01:04:50AM +0200, vtolkm@googlemail.com wrote:
>>> On 30/06/2019 00:49, Andrew Lunn wrote:
>>>> On Sun, Jun 30, 2019 at 12:01:38AM +0200, vtolkm@googlemail.com wrote:
>>>>> * DSA MV88E6060
>>>>> * iproute2 v.5.0.0-2.0
>>>>> * OpenWRT 19.07 with kernel 4.14.131 armv7l
>>>> The mv88e6060 driver is very simple. It has no support for VLANs. It
>>>> does not even have support for offloading bridging between ports to
>>>> the switch.
>>>>
>>>> The data sheet for this device is open. So if you want to hack on the
>>>> driver, you could do.
>>>>
>>>> Andrew
>>> Are you sure? That is a bit confusing after reading
>>> https://lore.kernel.org/patchwork/patch/575746/
>> Quoting that patch:
>>
>> This commit implements the switchdev operations to add, delete
>> and dump VLANs for the Marvell 88E6352 and compatible switch
>> chips.
>>
>> Vivien added support for the 6352. That uses the mv88e6xxx driver, not
>> the mv88e6060. And by compatible switches, he meant those in the 6352
>> family, so 6172 6176 6240 6352 and probably the 6171 6175 6350 6351.
>>
>> Andrew
> A simple soul might infer that mv88e6xxx includes MV88E6060, at least
> that happened to me apparently (being said simpleton).
> That may as it be, and pardon my continued ignorance, how is it
> explained then that a command as
>
> # bridge v a dev {bridge} self vid {n} untagged pvid
>
> reflects in
>
> # bridge v s
> a/o
> # bridge mo
>
> ?
>
> If the commands are not implemented one would expect them to fail in the
> first place or not reflecting a change at all?
>
>
As stated in the initial message - kernel conf
CONFIG_NET_DSA_MV88E6060=y
CONFIG_NET_DSA_MV88E6XXX=y
CONFIG_NET_DSA_MV88E6XXX_GLOBAL2=y
^ permalink raw reply
* Re: [PATCH rdma-next v4 06/17] RDMA/counter: Add "auto" configuration mode support
From: Jason Gunthorpe @ 2019-06-30 0:40 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Majd Dibbiny,
Mark Zhang, Saeed Mahameed, linux-netdev
In-Reply-To: <20190618172625.13432-7-leon@kernel.org>
On Tue, Jun 18, 2019 at 08:26:14PM +0300, Leon Romanovsky wrote:
> +static void __rdma_counter_dealloc(struct rdma_counter *counter)
> +{
> + mutex_lock(&counter->lock);
> + counter->device->ops.counter_dealloc(counter);
> + mutex_unlock(&counter->lock);
> +}
Does this lock do anything? The kref is 0 at this point, so no other
thread can have a pointer to this lock.
> +
> +static void rdma_counter_dealloc(struct rdma_counter *counter)
> +{
> + if (!counter)
> + return;
Counter is never NULL.
Jason
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: vtolkm @ 2019-06-30 1:13 UTC (permalink / raw)
To: netdev; +Cc: Andrew Lunn
In-Reply-To: <aa021a99-accd-75ac-47a4-2c11aa791d3c@gmail.com>
On 30/06/2019 02:37, vtolkm@gmail.com wrote:
> On 30/06/2019 01:23, vtolkm@gmail.com wrote:
>> On 30/06/2019 01:11, Andrew Lunn wrote:
>>> On Sun, Jun 30, 2019 at 01:04:50AM +0200, vtolkm@googlemail.com wrote:
>>>> On 30/06/2019 00:49, Andrew Lunn wrote:
>>>>> On Sun, Jun 30, 2019 at 12:01:38AM +0200, vtolkm@googlemail.com wrote:
>>>>>> * DSA MV88E6060
>>>>>> * iproute2 v.5.0.0-2.0
>>>>>> * OpenWRT 19.07 with kernel 4.14.131 armv7l
>>>>> The mv88e6060 driver is very simple. It has no support for VLANs. It
>>>>> does not even have support for offloading bridging between ports to
>>>>> the switch.
>>>>>
>>>>> The data sheet for this device is open. So if you want to hack on the
>>>>> driver, you could do.
>>>>>
>>>>> Andrew
>>>> Are you sure? That is a bit confusing after reading
>>>> https://lore.kernel.org/patchwork/patch/575746/
>>> Quoting that patch:
>>>
>>> This commit implements the switchdev operations to add, delete
>>> and dump VLANs for the Marvell 88E6352 and compatible switch
>>> chips.
>>>
>>> Vivien added support for the 6352. That uses the mv88e6xxx driver, not
>>> the mv88e6060. And by compatible switches, he meant those in the 6352
>>> family, so 6172 6176 6240 6352 and probably the 6171 6175 6350 6351.
>>>
>>> Andrew
>> A simple soul might infer that mv88e6xxx includes MV88E6060, at least
>> that happened to me apparently (being said simpleton).
>> That may as it be, and pardon my continued ignorance, how is it
>> explained then that a command as
>>
>> # bridge v a dev {bridge} self vid {n} untagged pvid
>>
>> reflects in
>>
>> # bridge v s
>> a/o
>> # bridge mo
>>
>> ?
>>
>> If the commands are not implemented one would expect them to fail in the
>> first place or not reflecting a change at all?
>>
>>
> As stated in the initial message - kernel conf
>
> CONFIG_NET_DSA_MV88E6060=y
> CONFIG_NET_DSA_MV88E6XXX=y
> CONFIG_NET_DSA_MV88E6XXX_GLOBAL2=y
>
Just figured that it is a Marvell 88E6176-TFJ2. Thus that cannot be the
cause, also considering the that the commands are executing and changes
being reflected.
However, the loss of access to the node is a mystery.
Apparently the filter is doing its job as in isolating access to the
bridge if connecting though an enslaved device.
And yet the bridge is still fully accessible from other devices that or
not enslaved by that bridge. As if the filter is inverted.
^ permalink raw reply
* Re: [PATCH v3 1/4] kbuild: compile-test UAPI headers to ensure they are self-contained
From: Masahiro Yamada @ 2019-06-30 1:27 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Linux Kbuild mailing list, Jani Nikula, Song Liu,
Alexei Starovoitov, Networking, Yonghong Song,
Linux Kernel Mailing List, linux-riscv, Michal Marek,
Martin KaFai Lau, Palmer Dabbelt, bpf, Daniel Borkmann, Albert Ou
In-Reply-To: <20190628154349.GA12826@ravnborg.org>
Hi Sam,
On Sat, Jun 29, 2019 at 12:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Masahiro.
>
> On Fri, Jun 28, 2019 at 01:38:59AM +0900, Masahiro Yamada wrote:
> > Multiple people have suggested compile-testing UAPI headers to ensure
> > they can be really included from user-space. "make headers_check" is
> > obviously not enough to catch bugs, and we often leak references to
> > kernel-space definition to user-space.
> >
> > Use the new header-test-y syntax to implement it. Please note exported
> > headers are compile-tested with a completely different set of compiler
> > flags. The header search path is set to $(objtree)/usr/include since
> > exported headers should not include unexported ones.
>
> This patchset introduce a new set of tests for uapi headers.
> Can we somehow consolidate so we have only one way to verify the uapi
> headers?
> It can be confusing for users that they see errors from testing the
> uapi headers during normal build and a new class or error if they
> run a "make headers_check" sometimes later.
Good point. I had also noticed some over-wrap
between this feature and scripts/headers_check.pl
For example, check_include will be unneeded.
I expect "make headers_check" will be deprecated
sooner or later.
> This can be a next step to consolidate this.
> With the suggestions below considered you can add my:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
> >
> > We use -std=gnu89 for the kernel space since the kernel code highly
> > depends on GNU extensions. On the other hand, UAPI headers should be
> > written in more standardized C, so they are compiled with -std=c90.
> > This will emit errors if C++ style comments, the keyword 'inline', etc.
> > are used. Please use C style comments (/* ... */), '__inline__', etc.
> > in UAPI headers.
> >
> > There is additional compiler requirement to enable this test because
> > many of UAPI headers include <stdlib.h>, <sys/ioctl.h>, <sys/time.h>,
> > etc. directly or indirectly. You cannot use kernel.org pre-built
> > toolchains [1] since they lack <stdlib.h>.
> >
> > I added scripts/cc-system-headers.sh to check the system header
> > availability, which CONFIG_UAPI_HEADER_TEST depends on.
> >
> > For now, a lot of headers need to be excluded because they cannot
> > be compiled standalone, but this is a good start point.
> >
> > [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/index.html
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > ---
> >
> > Changes in v3: None
> > Changes in v2:
> > - Add CONFIG_CPU_{BIG,LITTLE}_ENDIAN guard to avoid build error
> > - Use 'header-test-' instead of 'no-header-test'
> > - Avoid weird 'find' warning when cleaning
> >
> > Makefile | 2 +-
> > init/Kconfig | 11 +++
> > scripts/cc-system-headers.sh | 8 +++
> > usr/.gitignore | 1 -
> > usr/Makefile | 2 +
> > usr/include/.gitignore | 3 +
> > usr/include/Makefile | 134 +++++++++++++++++++++++++++++++++++
> > 7 files changed, 159 insertions(+), 2 deletions(-)
> > create mode 100755 scripts/cc-system-headers.sh
> > create mode 100644 usr/include/.gitignore
> > create mode 100644 usr/include/Makefile
> >
> > diff --git a/Makefile b/Makefile
> > index 1f35aca4fe05..f23516980796 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1363,7 +1363,7 @@ CLEAN_DIRS += $(MODVERDIR) include/ksym
> > CLEAN_FILES += modules.builtin.modinfo
> >
> > # Directories & files removed with 'make mrproper'
> > -MRPROPER_DIRS += include/config usr/include include/generated \
> > +MRPROPER_DIRS += include/config include/generated \
> > arch/$(SRCARCH)/include/generated .tmp_objdiff
> > MRPROPER_FILES += .config .config.old .version \
> > Module.symvers tags TAGS cscope* GPATH GTAGS GRTAGS GSYMS \
> > diff --git a/init/Kconfig b/init/Kconfig
> > index df5bba27e3fe..667d68e1cdf4 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -105,6 +105,17 @@ config HEADER_TEST
> > If you are a developer or tester and want to ensure the requested
> > headers are self-contained, say Y here. Otherwise, choose N.
> >
> > +config UAPI_HEADER_TEST
> > + bool "Compile test UAPI headers"
> > + depends on HEADER_TEST && HEADERS_INSTALL
> > + depends on $(success,$(srctree)/scripts/cc-system-headers.sh $(CC))
> > + help
> > + Compile test headers exported to user-space to ensure they are
> > + self-contained, i.e. compilable as standalone units.
> > +
> > + If you are a developer or tester and want to ensure the exported
> > + headers are self-contained, say Y here. Otherwise, choose N.
> > +
> > config LOCALVERSION
> > string "Local version - append to kernel release"
> > help
> > diff --git a/scripts/cc-system-headers.sh b/scripts/cc-system-headers.sh
> > new file mode 100755
> > index 000000000000..1b3db369828c
> > --- /dev/null
> > +++ b/scripts/cc-system-headers.sh
> > @@ -0,0 +1,8 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +cat << "END" | $@ -E -x c - -o /dev/null >/dev/null 2>&1
> > +#include <stdlib.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/time.h>
> > +END
>
> Add comment to this file that explains that it is used to verify that the
> toolchain provides the minimal set of header-files required by uapi
> headers.
Will do.
> > diff --git a/usr/.gitignore b/usr/.gitignore
> > index 8e48117a3f3d..be5eae1df7eb 100644
> > --- a/usr/.gitignore
> > +++ b/usr/.gitignore
> > @@ -7,4 +7,3 @@ initramfs_data.cpio.gz
> > initramfs_data.cpio.bz2
> > initramfs_data.cpio.lzma
> > initramfs_list
> > -include
> > diff --git a/usr/Makefile b/usr/Makefile
> > index 4a70ae43c9cb..6a89eb019275 100644
> > --- a/usr/Makefile
> > +++ b/usr/Makefile
> > @@ -56,3 +56,5 @@ $(deps_initramfs): klibcdirs
> > $(obj)/$(datafile_y): $(obj)/gen_init_cpio $(deps_initramfs) klibcdirs
> > $(Q)$(initramfs) -l $(ramfs-input) > $(obj)/$(datafile_d_y)
> > $(call if_changed,initfs)
> > +
> > +subdir-$(CONFIG_UAPI_HEADER_TEST) += include
> > diff --git a/usr/include/.gitignore b/usr/include/.gitignore
> > new file mode 100644
> > index 000000000000..a0991ff4402b
> > --- /dev/null
> > +++ b/usr/include/.gitignore
> > @@ -0,0 +1,3 @@
> > +*
> > +!.gitignore
> > +!Makefile
> > diff --git a/usr/include/Makefile b/usr/include/Makefile
> > new file mode 100644
> > index 000000000000..58ce96fa1701
> > --- /dev/null
> > +++ b/usr/include/Makefile
> > @@ -0,0 +1,134 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +# Unlike the kernel space, uapi headers are written in standard C.
> > +# - Forbid C++ style comments
> > +# - Use '__inline__', '__asm__' instead of 'inline', 'asm'
> > +#
> > +# -std=c90 (equivalent to -ansi) catches the violation of those.
> > +# We cannot go as far as adding -Wpedantic since it emits too many warnings.
> > +#
> > +# REVISIT: re-consider the proper set of compiler flags for uapi compile-test.
> > +
> > +UAPI_CFLAGS := -std=c90 -Wall -Werror=implicit-function-declaration
> > +
> > +override c_flags = $(UAPI_CFLAGS) -Wp,-MD,$(depfile) -I$(objtree)/usr/include
> > +
> > +# The following are excluded for now because they fail to build.
> > +# The cause of errors are mostly missing include directives.
> > +# Check one by one, and send a patch to each subsystem.
> > +#
> > +# Do not add a new header to the blacklist without legitimate reason.
> > +# Please consider to fix the header first.
>
> Maybe add comment that the alphabetical sort by filename must be preserved.
> Not too relevant, as we hopefully do not see files being added.
>
> > +header-test- += asm/ipcbuf.h
> > +header-test- += asm/msgbuf.h
> Consider same syntax like in include/Makefile where you use
> header-test-<tab><tab>...+= file
>
> Then the alignment looks betters.
Probably I can do that, but this is not so important
because our ultimate goal is (almost) no blacklist.
> > +
> > +# The rest are compile-tested
> > +header-test-y += $(filter-out $(header-test-), \
> > + $(patsubst $(obj)/%,%, $(wildcard \
> > + $(addprefix $(obj)/, *.h */*.h */*/*.h */*/*/*.h))))
> Could you use header-test-pattern-y here?
header-test-pattern-y does not work here
because it only matches to $(srctree)/$(src)/*
All headers under usr/include/
are generated one, and located in $(objtree)/$(obj)/.
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v3 1/4] kbuild: compile-test UAPI headers to ensure they are self-contained
From: Masahiro Yamada @ 2019-06-30 1:35 UTC (permalink / raw)
To: Linux Kbuild mailing list
Cc: Song Liu, Michal Marek, bpf, Daniel Borkmann, Jani Nikula,
Networking, Palmer Dabbelt, Alexei Starovoitov,
Linux Kernel Mailing List, Albert Ou, Yonghong Song, linux-riscv,
Sam Ravnborg, Martin KaFai Lau
In-Reply-To: <20190627163903.28398-2-yamada.masahiro@socionext.com>
On Fri, Jun 28, 2019 at 1:40 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Multiple people have suggested compile-testing UAPI headers to ensure
> they can be really included from user-space. "make headers_check" is
> obviously not enough to catch bugs, and we often leak references to
> kernel-space definition to user-space.
>
> Use the new header-test-y syntax to implement it. Please note exported
> headers are compile-tested with a completely different set of compiler
> flags. The header search path is set to $(objtree)/usr/include since
> exported headers should not include unexported ones.
>
> We use -std=gnu89 for the kernel space since the kernel code highly
> depends on GNU extensions. On the other hand, UAPI headers should be
> written in more standardized C, so they are compiled with -std=c90.
> This will emit errors if C++ style comments, the keyword 'inline', etc.
> are used. Please use C style comments (/* ... */), '__inline__', etc.
> in UAPI headers.
>
> There is additional compiler requirement to enable this test because
> many of UAPI headers include <stdlib.h>, <sys/ioctl.h>, <sys/time.h>,
> etc. directly or indirectly. You cannot use kernel.org pre-built
> toolchains [1] since they lack <stdlib.h>.
>
> I added scripts/cc-system-headers.sh to check the system header
> availability, which CONFIG_UAPI_HEADER_TEST depends on.
Perhaps, we could use scripts/cc-can-link.sh for this purpose.
The intention is slightly different, but a compiler to link
user-space programs must provide necessary standard headers.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Yonghong Song @ 2019-06-30 5:52 UTC (permalink / raw)
To: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org
Cc: davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
Andrii Nakryiko, kernel test robot
In-Reply-To: <20190628231049.22149-1-sdf@google.com>
On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
> that it can do a single u64 store into user_ip6[2] instead of two
> separate u32 ones:
>
> # 17: (18) r2 = 0x100000000000000
> # ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
> # 19: (7b) *(u64 *)(r1 +16) = r2
> # invalid bpf_context access off=16 size=8
>
> From the compiler point of view it does look like a correct thing
> to do, so let's support it on the kernel side.
>
> Credit to Andrii Nakryiko for a proper implementation of
> bpf_ctx_wide_store_ok.
>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
The change looks good to me with the following nits:
1. could you add a cover letter for the patch set?
typically if the number of patches is more than one,
it would be a good practice with a cover letter.
See bpf_devel_QA.rst .
2. with this change, the comments in uapi bpf.h
are not accurate any more.
__u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
* Stored in network byte order.
*/
__u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
* Stored in network byte order.
*/
now for stores, aligned 8-byte write is permitted.
could you update this as well?
From the typical usage pattern, I did not see a need
for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
us just deal with write for now.
With the above two nits,
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/filter.h | 6 ++++++
> net/core/filter.c | 22 ++++++++++++++--------
> 2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 340f7d648974..3901007e36f1 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
> return size <= size_default && (size & (size - 1)) == 0;
> }
>
> +#define bpf_ctx_wide_store_ok(off, size, type, field) \
> + (size == sizeof(__u64) && \
> + off >= offsetof(type, field) && \
> + off + sizeof(__u64) <= offsetofend(type, field) && \
> + off % sizeof(__u64) == 0)
> +
> #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
>
> static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index dc8534be12fc..5d33f2146dab 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
> if (!bpf_ctx_narrow_access_ok(off, size, size_default))
> return false;
> } else {
> + if (bpf_ctx_wide_store_ok(off, size,
> + struct bpf_sock_addr,
> + user_ip6))
> + return true;
> +
> + if (bpf_ctx_wide_store_ok(off, size,
> + struct bpf_sock_addr,
> + msg_src_ip6))
> + return true;
> +
> if (size != size_default)
> return false;
> }
> @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
> * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
> *
> - * It doesn't support SIZE argument though since narrow stores are not
> - * supported for now.
> - *
> * In addition it uses Temporary Field TF (member of struct S) as the 3rd
> * "register" since two registers available in convert_ctx_access are not
> * enough: we can't override neither SRC, since it contains value to store, nor
> @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> * instructions. But we need a temporary place to save pointer to nested
> * structure whose field we want to store to.
> */
> -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF) \
> +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF) \
> do { \
> int tmp_reg = BPF_REG_9; \
> if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg) \
> @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> offsetof(S, TF)); \
> *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
> si->dst_reg, offsetof(S, F)); \
> - *insn++ = BPF_STX_MEM( \
> - BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg, \
> + *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
> bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF), \
> target_size) \
> + OFF); \
> @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> TF) \
> do { \
> if (type == BPF_WRITE) { \
> - SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, \
> - TF); \
> + SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, \
> + OFF, TF); \
> } else { \
> SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF( \
> S, NS, F, NF, SIZE, OFF); \
>
^ 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