Netdev List
 help / color / mirror / Atom feed
* Re: brcmfmac firmware issue on NanoPi K2
From: Franky Lin @ 2017-05-22 21:39 UTC (permalink / raw)
  To: Andreas Färber
  Cc: linux-wireless,
	open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER, netdev,
	technicalsupport, Arend Van Spriel, linux-amlogic
In-Reply-To: <ab713116-ab12-5f87-1ecf-cae2e0d68b93@suse.de>

Hi Andreas,

On Sun, May 21, 2017 at 9:20 AM, Andreas Färber <afaerber@suse.de> wrote:
> Hello,
>
> The NanoPi K2 has an Ampak AP6212 SDIO module. brcmfmac driver loads
> brcmfmac43430-sdio.bin.
>
> When using the firmware file from linux-firmware.git that openSUSE ships
> I get the following errors on 4.11.0 and next-20170519:
>
> [ 2103.618716] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000):
> clkctl 0x50
> [ 2104.668746] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000):
> clkctl 0x50
> [ 2105.678677] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000):
> clkctl 0x50
>
> If I overwrite /lib/firmware/brcm/bcm43430-sdio.bin with
> fw_bcm43438a0.bin from FriendlyARM's Android repository it suddenly works:
>
> [  +0.157738] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0:
> Jun  6 2014 14:50:39 version 7.10.226.49 (r) FWID 01-8962686a
> [  +0.160108] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166 code
> (0x30 0x30)
>
> I recall using the linux-firmware.git brcmfmac43430-sdio.bin file
> successfully on the Raspberry Pi 3 with a downstream (Leap 42.2) kernel.
>
> I've tested both nvram_ap6212.txt and nvram_ap6212a.txt, the latter has
> the following diff to nvram.txt:
>
> --- nvram_ap6212.txt    2017-05-21 04:24:40.372113426 +0200
> +++ nvram_ap6212a.txt   2017-05-21 04:24:49.852116599 +0200
> @@ -1,4 +1,4 @@
> -#AP6212_NVRAM_V1.0_20140603
> +#AP6212_NVRAM_V1.0.1_20160606
>  # 2.4 GHz, 20 MHz BW mode
>
>  # The following parameter values are just placeholders, need to be updated.
> @@ -51,4 +51,4 @@
>  muxenab=0x10
>  # CLDO PWM voltage settings - 0x4 - 1.1 volt
>  #cldo_pwm=0x4
> -
> +glitch_based_crsmin=1
>
> https://github.com/friendlyarm/android_hardware_amlogic_wifi/tree/l-amlogic-gx-sync/bcm_ampak/config/6212
>
> * Does the linux-firmware.git brcmfmac43430-sdio.bin need a fix for AP6212?
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm
>
> * Does the brcmfmac driver need to distinguish revisions in sdio.c as
> done for 43241, plus a separate firmware file?
>         BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0xFFFFFFFF, 43430),

It seems we are dealing with different revisions of 43430. The
firmware file you pointed to is for 43430a0 and raspberry pi 3 is
using 43430a1. So yes brcmfmac needs to load different firmware and
nvram for them just like 43241.

Could you please help try the attached patch? I don't have the
hardware to test it. Please rename the 43438 firmware and nvram to
brcmfmac43430a0-sdio.bin/txt.

Thanks,
Franky

---

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index a999f95..1b12ccb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -608,6 +608,7 @@ BRCMF_FW_NVRAM_DEF(43340,
"brcmfmac43340-sdio.bin", "brcmfmac43340-sdio.txt");
 BRCMF_FW_NVRAM_DEF(4335, "brcmfmac4335-sdio.bin", "brcmfmac4335-sdio.txt");
 BRCMF_FW_NVRAM_DEF(43362, "brcmfmac43362-sdio.bin", "brcmfmac43362-sdio.txt");
 BRCMF_FW_NVRAM_DEF(4339, "brcmfmac4339-sdio.bin", "brcmfmac4339-sdio.txt");
+BRCMF_FW_NVRAM_DEF(43430A0, "brcmfmac43430a0-sdio.bin",
"brcmfmac43430a0-sdio.txt");
 BRCMF_FW_NVRAM_DEF(43430, "brcmfmac43430-sdio.bin", "brcmfmac43430-sdio.txt");
 BRCMF_FW_NVRAM_DEF(43455, "brcmfmac43455-sdio.bin", "brcmfmac43455-sdio.txt");
 BRCMF_FW_NVRAM_DEF(4354, "brcmfmac4354-sdio.bin", "brcmfmac4354-sdio.txt");
@@ -626,7 +627,8 @@ static struct brcmf_firmware_mapping
brcmf_sdio_fwnames[] = {
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4335_CHIP_ID, 0xFFFFFFFF, 4335),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43362_CHIP_ID, 0xFFFFFFFE, 43362),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4339_CHIP_ID, 0xFFFFFFFF, 4339),
- BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0xFFFFFFFF, 43430),
+ BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0x00000001, 43430A0),
+ BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0xFFFFFFFE, 43430),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4345_CHIP_ID, 0xFFFFFFC0, 43455),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4354_CHIP_ID, 0xFFFFFFFF, 4354),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4356_CHIP_ID, 0xFFFFFFFF, 4356)

^ permalink raw reply related

* Re: network performance degradation in virtio_net in 4.12-rc
From: Mikulas Patocka @ 2017-05-22 21:50 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20170522203532-mutt-send-email-mst@kernel.org>



On Mon, 22 May 2017, Michael S. Tsirkin wrote:

> On Mon, May 22, 2017 at 10:25:19AM -0400, Mikulas Patocka wrote:
> > Hi
> > 
> > I see severe network performance degradation with the kernels 4.12-rc1 and 
> > 4.12-rc2 in the network virtio driver. Download rate drops down to about 
> > 100kB/s.
> > 
> > I bisected it and it is caused by patch 
> > d85b758f72b05a774045545f24d70980e3e9aac4 ("virtio_net: fix support for 
> > small rings"). When I revert this patch, the problem goes away.
> > 
> > The host is Debian Jessie with kernel 4.4.62, the guest is Debian Sid with 
> > kernel 4.12-rc.
> > 
> > Mikulas
> 
> Thanks for the report!
> I'm not sure what causes it on this host. I'll try to reproduce.
> Meanwhile, could you please locate this line of code:
> +               vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);
> 
> and add something like
> 	printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
> 	       vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
> 	       virtqueue_get_vring_size(vi->rq[i].vq),
> 	       (int)vi->big_packets);
> 
> after it?
> Then boot and capture the output.
> 
> Thanks!

It prints one line "min buf = 0x101 expected 0x5ee size 0x100 big 1".

Mikulas

^ permalink raw reply

* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Florian Fainelli @ 2017-05-22 21:50 UTC (permalink / raw)
  To: Timur Tabi, Andrew Lunn
  Cc: Zefir Kurtisi, netdev, David Miller, Manoj Iyer, jhugo
In-Reply-To: <c57adeeb-b51c-5ac8-776e-1ead474f7b48@codeaurora.org>

On 05/22/2017 02:19 PM, Timur Tabi wrote:
> On 05/22/2017 04:10 PM, Florian Fainelli wrote:
>> Even a module argument would be rejected. If you need platform/SoC
>> specific behavior propagated down to the PHY driver, several options exist:
>>
>> - pass an agreed upon value for phy_flags to of_phy_connect() see
>> drivers/net/ethernet/broadcom/tg3.c and
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c for instance and update
>> the driver to act on that "flags" see drivers/net/phy/broadcom.c and
>> drivers/net/phy/bcm7xxx.c
> 
> Will this work on ACPI systems as well?

Provided you get a reference on the PHY dvice first, yes.

>  I call phy_connect_direct() instead
> of of_phy_connect().  I see some drivers set phydev->dev_flags before
> calling phy_connect_direct().

Setting phydev->dev_flags before calling phy_connect_direct() is okay
and will work here. The key thing is that you will need to get a PHY
device reference before, which would already be populated in your MDIO
bus's mdio_map array, see e.g: mdiobus_get_phy().

You can also set phydev->dev_flags *after* calling phy_connect_direct().
The only reason why it should be done before, is to guarantee that
phydrv::config_init would *see* the correct value there. If you need it
at a later time, like in config_aneg() or aneg_done(), setting it later
*might work*, but you'd have to trigger a auto-negotiation restart to
avoid races between phy_connect_direct() returning, and
phydrv::config_aneg() being called.

> 
> My concern is that this problem occurs only on an at8031 chip, so having my
> network driver passing an at8031-specific flag seems out of place.  What
> happens if, on some other board, a different PHY is used, and that flag
> means something else?

There needs to be an agreement on what the flags bits mean, and this
needs to be in a shared header file that other network drivers can
reference and where they can allocate their own bits if existing
functionality is not covered. The allocation of such flags is centered
around the perspective of the PHY driver.

Of course, this only works if you have a strict mapping between the
Ethernet MAC and the PHY, and if your MAC only uses the same PHY device
driver. If that's not the case, you need to make sure you don't pass a
phy_flags with bits set that could influence the behavior of another PHY
driver that would also try to do something with these phy_flags. Fairly
positive you can figure this out from your Ethernet MAC driver.

> 
>> - register a PHY fixup which is specific to the board/SoC, and have the
>> PHY fixup do whatever is necessary for your platform (like setting
>> specific registers)
> 
> Do you have an example of that?

You could have grepped for phy_register_fixup() but the best example I
can come up with is:

drivers/net/usb/lan78xx.c

and there are a lot more in arch/{arm,powerpc}/:

arch/arm/mach-davinci/board-dm644x-evm.c:
phy_register_fixup_for_uid(LXT971_PHY_ID, LXT971_PHY_MASK,
arch/arm/mach-imx/mach-imx6q.c:
phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
arch/arm/mach-imx/mach-imx6q.c:
phy_register_fixup_for_uid(PHY_ID_KSZ9031, MICREL_PHY_ID_MASK,
arch/arm/mach-imx/mach-imx6q.c:
phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffef,
arch/arm/mach-imx/mach-imx6q.c:
phy_register_fixup_for_uid(PHY_ID_AR8035, 0xffffffef,
arch/arm/mach-imx/mach-imx6sx.c:
phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffff,
arch/arm/mach-imx/mach-imx6ul.c:
phy_register_fixup_for_uid(PHY_ID_KSZ8081, MICREL_PHY_ID_MASK,
arch/arm/mach-imx/mach-imx7d.c:
phy_register_fixup_for_uid(PHY_ID_AR8031, 0xffffffff,
arch/arm/mach-imx/mach-imx7d.c:
phy_register_fixup_for_uid(PHY_ID_BCM54220, 0xffffffff,
arch/arm/mach-mxs/mach-mxs.c:
phy_register_fixup_for_uid(PHY_ID_KSZ8051, MICREL_PHY_ID_MASK,
arch/arm/mach-orion5x/dns323-setup.c:
phy_register_fixup_for_uid(MARVELL_PHY_ID_88E1118,
arch/powerpc/platforms/85xx/mpc85xx_mds.c:
phy_register_fixup_for_id(phy_id, mpc8568_fixup_125_clock);
arch/powerpc/platforms/85xx/mpc85xx_mds.c:
phy_register_fixup_for_id(phy_id, mpc8568_mds_phy_fixups);
arch/powerpc/platforms/85xx/mpc85xx_mds.c:
phy_register_fixup_for_id(phy_id, mpc8568_mds_phy_fixups);
-- 
Florian

^ permalink raw reply

* Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument
From: Kees Cook @ 2017-05-22 22:20 UTC (permalink / raw)
  To: Djalal Harouni
  Cc: LKML, Network Development, linux-security-module,
	kernel-hardening@lists.openwall.com, Andy Lutomirski,
	Andrew Morton, Rusty Russell, Serge E. Hallyn, Jessica Yu,
	David S. Miller, James Morris, Paul Moore, Stephen Smalley,
	Greg Kroah-Hartman, Tetsuo Handa, Ingo Molnar, Linux API,
	Dongsu Park
In-Reply-To: <1495454226-10027-2-git-send-email-tixxdz@gmail.com>

On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> This is a preparation patch for the module auto-load restriction feature.
>
> In order to restrict module auto-load operations we need to check if the
> caller has CAP_SYS_MODULE capability. This allows to align security
> checks of automatic module loading with the checks of the explicit operations.
>
> However for "netdev-%s" modules, they are allowed to be loaded if
> CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
> and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
> capability which is considered a privileged operation, we have two
> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
> the capability form request_module() to security_kernel_module_request()
> hook and let the capability subsystem decide.
>
> After a discussion with Rusty Russell [1], the suggestion was to pass
> the capability from request_module() to security_kernel_module_request()
> for 'netdev-%s' modules that need CAP_NET_ADMIN.
>
> The patch does not update request_module(), it updates the internal
> __request_module() that will take an extra "allow_cap" argument. If
> positive, then automatic module load operation can be allowed.

I find this refactor slightly confusing. I would expect to collapse
the existing caps checks in net/core/dev_ioctl.c and
net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
add a new non-__ function instead of requiring callers use
__request_module.

request_module_capable(int cap_required, fmt, args);

adjust __request_module() for the new arg, and when cap_required !=
-1, perform a cap check.

Then make request_module pass -1 to __request_module(), and change
dev_ioctl.c (and tcp_cong.c) from:

        if (no_module && capable(CAP_NET_ADMIN))
                no_module = request_module("netdev-%s", name);
        if (no_module && capable(CAP_SYS_MODULE))
                request_module("%s", name);

to:

        if (no_module)
                no_module = request_module_capable(CAP_NET_ADMIN,
"netdev-%s", name);
        if (no_module)
                no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);

that'll make the code cleaner, too.

> __request_module() will be only called by networking code which is the
> exception to this, so we do not break userspace and CAP_NET_ADMIN can
> continue to load 'netdev-%s' modules. Other kernel code should continue
> to use request_module() which calls security_kernel_module_request() and
> will check for CAP_SYS_MODULE capability in next patch. Allowing more
> control on who can trigger automatic module loading.
>
> This patch updates security_kernel_module_request() to take the
> 'allow_cap' argument and SELinux which is currently the only user of
> security_kernel_module_request() hook.
>
> Based on patch by Rusty Russell:
> https://lkml.org/lkml/2017/4/26/735
>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Suggested-by: Rusty Russell <rusty@rustcorp.com.au>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>
> [1] https://lkml.org/lkml/2017/4/24/7
> ---
>  include/linux/kmod.h      | 15 ++++++++-------
>  include/linux/lsm_hooks.h |  4 +++-
>  include/linux/security.h  |  4 ++--
>  kernel/kmod.c             | 15 +++++++++++++--
>  net/core/dev_ioctl.c      | 10 +++++++++-
>  security/security.c       |  4 ++--
>  security/selinux/hooks.c  |  2 +-
>  7 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index c4e441e..a314432 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -32,18 +32,19 @@
>  extern char modprobe_path[]; /* for sysctl */
>  /* modprobe exit status on success, -ve on error.  Return value
>   * usually useless though. */
> -extern __printf(2, 3)
> -int __request_module(bool wait, const char *name, ...);
> -#define request_module(mod...) __request_module(true, mod)
> -#define request_module_nowait(mod...) __request_module(false, mod)
> +extern __printf(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...);
>  #define try_then_request_module(x, mod...) \
> -       ((x) ?: (__request_module(true, mod), (x)))
> +       ((x) ?: (__request_module(true, -1, mod), (x)))
>  #else
> -static inline int request_module(const char *name, ...) { return -ENOSYS; }
> -static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
> +static inline __printf(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...)
> +{ return -ENOSYS; }
>  #define try_then_request_module(x, mod...) (x)
>  #endif
>
> +#define request_module(mod...) __request_module(true, -1, mod)
> +#define request_module_nowait(mod...) __request_module(false, -1, mod)
>
>  struct cred;
>  struct file;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f7914d9..7688f79 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -578,6 +578,8 @@
>   *     Ability to trigger the kernel to automatically upcall to userspace for
>   *     userspace to load a kernel module with the given name.
>   *     @kmod_name name of the module requested by the kernel
> + *     @allow_cap capability that allows to automatically load a kernel
> + *     module.

I would describe this as "required to load".

>   *     Return 0 if successful.
>   * @kernel_read_file:
>   *     Read a file specified by userspace.
> @@ -1516,7 +1518,7 @@ union security_list_options {
>         void (*cred_transfer)(struct cred *new, const struct cred *old);
>         int (*kernel_act_as)(struct cred *new, u32 secid);
>         int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
> -       int (*kernel_module_request)(char *kmod_name);
> +       int (*kernel_module_request)(char *kmod_name, int allow_cap);
>         int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
>         int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>                                      enum kernel_read_file_id id);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 549cb82..2f4c9d3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,7 +325,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred *old);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> -int security_kernel_module_request(char *kmod_name);
> +int security_kernel_module_request(char *kmod_name, int allow_cap);
>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>                                    enum kernel_read_file_id id);
> @@ -926,7 +926,7 @@ static inline int security_kernel_create_files_as(struct cred *cred,
>         return 0;
>  }
>
> -static inline int security_kernel_module_request(char *kmod_name)
> +static inline int security_kernel_module_request(char *kmod_name, int allow_cap)
>  {
>         return 0;
>  }
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 563f97e..15c96e8 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -110,6 +110,7 @@ static int call_modprobe(char *module_name, int wait)
>  /**
>   * __request_module - try to load a kernel module
>   * @wait: wait (or not) for the operation to complete
> + * @allow_cap: if positive, may allow modprobe if this capability is set.
>   * @fmt: printf style format string for the name of the module
>   * @...: arguments as specified in the format string
>   *
> @@ -120,10 +121,20 @@ static int call_modprobe(char *module_name, int wait)
>   * must check that the service they requested is now available not blindly
>   * invoke it.
>   *
> + * If "allow_cap" is positive, The security subsystem will trust the caller
> + * that "allow_cap" may allow to load some modules with a specific alias,
> + * the security subsystem will make some exceptions based on that. This is
> + * primally useful for backward compatibility. A permission check should not
> + * be that strict and userspace should be able to continue to trigger module
> + * auto-loading if needed.
> + *
>   * If module auto-loading support is disabled then this function
>   * becomes a no-operation.
> + *
> + * This function should not be directly used by other subsystems, for that
> + * please call request_module().
>   */
> -int __request_module(bool wait, const char *fmt, ...)
> +int __request_module(bool wait, int allow_cap, const char *fmt, ...)
>  {
>         va_list args;
>         char module_name[MODULE_NAME_LEN];
> @@ -150,7 +161,7 @@ int __request_module(bool wait, const char *fmt, ...)
>         if (ret >= MODULE_NAME_LEN)
>                 return -ENAMETOOLONG;
>
> -       ret = security_kernel_module_request(module_name);
> +       ret = security_kernel_module_request(module_name, allow_cap);
>         if (ret)
>                 return ret;
>
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index b94b1d2..c494351 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -366,8 +366,16 @@ void dev_load(struct net *net, const char *name)
>         rcu_read_unlock();
>
>         no_module = !dev;
> +       /*
> +        * First do the CAP_NET_ADMIN check, then let the security
> +        * subsystem checks know that this can be allowed since this is
> +        * a "netdev-%s" module and CAP_NET_ADMIN is set.
> +        *
> +        * For this exception call __request_module().
> +        */
>         if (no_module && capable(CAP_NET_ADMIN))
> -               no_module = request_module("netdev-%s", name);
> +               no_module = __request_module(true, CAP_NET_ADMIN,
> +                                            "netdev-%s", name);
>         if (no_module && capable(CAP_SYS_MODULE))
>                 request_module("%s", name);
>  }
> diff --git a/security/security.c b/security/security.c
> index 714433e..cedb790 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1021,9 +1021,9 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
>         return call_int_hook(kernel_create_files_as, 0, new, inode);
>  }
>
> -int security_kernel_module_request(char *kmod_name)
> +int security_kernel_module_request(char *kmod_name, int allow_cap)
>  {
> -       return call_int_hook(kernel_module_request, 0, kmod_name);
> +       return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
>  }
>
>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 158f6a0..85eeff6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3842,7 +3842,7 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
>         return ret;
>  }
>
> -static int selinux_kernel_module_request(char *kmod_name)
> +static int selinux_kernel_module_request(char *kmod_name, int allow_cap)
>  {
>         struct common_audit_data ad;
>
> --
> 2.10.2
>

Otherwise, looks good!

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply

* RE: [PATCH v3 net-next 3/5] dsa: add DSA switch driver for Microchip KSZ9477
From: Woojung.Huh @ 2017-05-22 22:26 UTC (permalink / raw)
  To: andrew
  Cc: f.fainelli, vivien.didelot, sergei.shtylyov, netdev, davem,
	UNGLinuxDriver
In-Reply-To: <20170522212757.GS29447@lunn.ch>

> > > > +static int get_vlan_table(struct dsa_switch *ds, u16 vid, u32
> *vlan_table)
> > > > +{
> > > > +	struct ksz_device *dev = ds->priv;
> > > > +	u8 data;
> > > > +	int timeout = 1000;
> > > > +
> > > > +	ksz_write16(dev, REG_SW_VLAN_ENTRY_INDEX__2, vid &
> > > VLAN_INDEX_M);
> > > > +	ksz_write8(dev, REG_SW_VLAN_CTRL, VLAN_READ | VLAN_START);
> > > > +
> > > > +	/* wait to be cleared */
> > > > +	data = 0;
> > > > +	do {
> > > > +		ksz_read8(dev, REG_SW_VLAN_CTRL, &data);
> > > > +		if (!(data & VLAN_START))
> > > > +			break;
> > > > +		usleep_range(1, 10);
> > > > +	} while (timeout-- > 0);
> > > > +
> > > > +	if (!timeout)
> > > > +		return -ETIMEDOUT;
> > > > +
> > > > +	ksz_read32(dev, REG_SW_VLAN_ENTRY__4, &vlan_table[0]);
> > > > +	ksz_read32(dev, REG_SW_VLAN_ENTRY_UNTAG__4,
> > > &vlan_table[1]);
> > > > +	ksz_read32(dev, REG_SW_VLAN_ENTRY_PORTS__4, &vlan_table[2]);
> > > > +
> > > > +	ksz_write8(dev, REG_SW_VLAN_CTRL, 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int set_vlan_table(struct dsa_switch *ds, u16 vid, u32
> *vlan_table)
> > > > +{
> > > > +	struct ksz_device *dev = ds->priv;
> > > > +	u8 data;
> > > > +	int timeout = 1000;
> > > > +
> > > > +	ksz_write32(dev, REG_SW_VLAN_ENTRY__4, vlan_table[0]);
> > > > +	ksz_write32(dev, REG_SW_VLAN_ENTRY_UNTAG__4, vlan_table[1]);
> > > > +	ksz_write32(dev, REG_SW_VLAN_ENTRY_PORTS__4, vlan_table[2]);
> > > > +
> > > > +	ksz_write16(dev, REG_SW_VLAN_ENTRY_INDEX__2, vid &
> > > VLAN_INDEX_M);
> > > > +	ksz_write8(dev, REG_SW_VLAN_CTRL, VLAN_START | VLAN_WRITE);
> > > > +
> > > > +	do {
> > > > +		ksz_read8(dev, REG_SW_VLAN_CTRL, &data);
> > > > +		if (!(data & VLAN_START))
> > > > +			break;
> > > > +		usleep_range(1, 10);
> > > > +	} while (timeout-- > 0);
> > > > +
> > > > +	if (!timeout)
> > > > +		return -ETIMEDOUT;
> > > > +
> > > > +	ksz_write8(dev, REG_SW_VLAN_CTRL, 0);
> > > > +
> > > > +	mutex_lock(&dev->vlancache_mutex);
> > >
> > > Humm. I think this is wrong. Shouldn't you hold the mutex while you
> > > change the hardware as well as the cache. Otherwise there is a risk
> > > your cache could be different to the hardware when you get a race
> > > between two threads?
> > Thanks for pointing this out.
> > Rather than two separate mutex (H/W and vlancache), will put one HW
> access mutex
> > around get_vlan_table and set_vlan_table to cover vlancache access too.
> Even though
> > little bit overhead. How do you think?
> 
> I would move the mutex_lock(&dev->vlancache_mutex) to be beginning of
> the function. It then protects both the hardware and the vlan cache,
> and keeps them synchronised.
> 
Andrew,

I believe mutex is needed in get_vlan_table() too.
vlancache_mutex doesn't match exactly what it does, I would change name to
vlan_mutex.

Thanks.
- Woojung

^ permalink raw reply

* Re: [PATCH v4 next 2/3] modules:capabilities: automatic module loading restriction
From: Kees Cook @ 2017-05-22 22:28 UTC (permalink / raw)
  To: Djalal Harouni
  Cc: LKML, Network Development, linux-security-module,
	kernel-hardening@lists.openwall.com, Andy Lutomirski,
	Andrew Morton, Rusty Russell, Serge E. Hallyn, Jessica Yu,
	David S. Miller, James Morris, Paul Moore, Stephen Smalley,
	Greg Kroah-Hartman, Tetsuo Handa, Ingo Molnar, Linux API,
	Dongsu Park
In-Reply-To: <1495454226-10027-3-git-send-email-tixxdz@gmail.com>

On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> [...]
> diff --git a/kernel/module.c b/kernel/module.c
> index 4a3665f..ce7a146 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -282,6 +282,8 @@ module_param(sig_enforce, bool_enable_only, 0644);
>
>  /* Block module loading/unloading? */
>  int modules_disabled = 0;
> +int modules_autoload_mode = MODULES_AUTOLOAD_ALLOWED;
> +const int modules_autoload_max = MODULES_AUTOLOAD_DISABLED;
>  core_param(nomodule, modules_disabled, bint, 0);
>
>  /* Waiting for a module to finish initializing? */
> @@ -4296,6 +4298,46 @@ struct module *__module_text_address(unsigned long addr)
>  }
>  EXPORT_SYMBOL_GPL(__module_text_address);
>
> +/**
> + * may_autoload_module - Determine whether a module auto-load operation
> + * is permitted
> + * @kmod_name: The module name
> + * @allow_cap: if positive, may allow to auto-load the module if this capability
> + * is set
> + *
> + * Determine whether a module auto-load operation is allowed or not. The check
> + * uses the sysctl "modules_autoload_mode" value.
> + *
> + * This allows to have more control on automatic module loading, and align it
> + * with explicit load/unload module operations. The kernel contains several
> + * modules, some of them are not updated often and may contain bugs and
> + * vulnerabilities.
> + *
> + * The "allow_cap" is passed by callers to explicitly note that the module has
> + * the appropriate alias and that the "allow_cap" capability is set. This is
> + * for backward compatibility, the aim is to have a clear picture where:
> + *
> + * 1) Implicit module loading is allowed
> + * 2) Implicit module loading as with the explicit one requires CAP_SYS_MODULE.
> + * 3) Implicit module loading as with the explicit one can be disabled.
> + *
> + * Returns 0 if the module request is allowed or -EPERM if not.
> + */
> +int may_autoload_module(char *kmod_name, int allow_cap)
> +{
> +       if (modules_autoload_mode == MODULES_AUTOLOAD_ALLOWED)
> +               return 0;
> +       else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
> +               /* Check CAP_SYS_MODULE then allow_cap if valid */
> +               if (capable(CAP_SYS_MODULE) ||
> +                   (allow_cap > 0 && capable(allow_cap)))

With the allow_cap check already happening in my suggestion for
__request_module(), it's not needed here. (In fact, it's not even
really needed to plumb this into the hook, I don't think?

Regardless, I remain a fan. :)

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply

* [PATCH net-next 1/2] perf, bpf: add support for HW_CACHE and RAW events
From: Alexei Starovoitov @ 2017-05-22 22:48 UTC (permalink / raw)
  To: David S . Miller
  Cc: Peter Zijlstra, Brendan Gregg, Daniel Borkmann, Teng Qin, netdev,
	linux-kernel, kernel-team
In-Reply-To: <20170522224840.810121-1-ast@fb.com>

From: Teng Qin <qinteng@fb.com>

This commit adds support for attach BPF program to RAW and HW_CACHE type
events, and support for read HW_CACHE type event counters in BPF
program. Existing code logic already supports them, so this commit is
just update Enum value checks.

Signed-off-by: Teng Qin <qinteng@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/arraymap.c    | 1 +
 kernel/events/core.c     | 4 +++-
 kernel/trace/bpf_trace.c | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 5e00b2333c26..f32affe8c335 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -473,6 +473,7 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map,
 			goto err_out;
 		/* fall-through */
 	case PERF_TYPE_RAW:
+	case PERF_TYPE_HW_CACHE:
 	case PERF_TYPE_HARDWARE:
 		ee = bpf_event_entry_gen(perf_file, map_file);
 		if (ee)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6e75a5c9412d..1b68cb751c03 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8038,7 +8038,9 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
 	struct bpf_prog *prog;
 
 	if (event->attr.type == PERF_TYPE_HARDWARE ||
-	    event->attr.type == PERF_TYPE_SOFTWARE)
+	    event->attr.type == PERF_TYPE_SOFTWARE ||
+	    event->attr.type == PERF_TYPE_HW_CACHE ||
+	    event->attr.type == PERF_TYPE_RAW)
 		return perf_event_set_bpf_handler(event, prog_fd);
 
 	if (event->attr.type != PERF_TYPE_TRACEPOINT)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 460a031c77e5..5cbda7962a32 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -249,6 +249,7 @@ BPF_CALL_2(bpf_perf_event_read, struct bpf_map *, map, u64, flags)
 
 	event = ee->event;
 	if (unlikely(event->attr.type != PERF_TYPE_HARDWARE &&
+		     event->attr.type != PERF_TYPE_HW_CACHE &&
 		     event->attr.type != PERF_TYPE_RAW))
 		return -EINVAL;
 
-- 
2.9.3

^ permalink raw reply related

* [PATCH net-next 2/2] samples/bpf: add samples for HW_CACHE / RAW events
From: Alexei Starovoitov @ 2017-05-22 22:48 UTC (permalink / raw)
  To: David S . Miller
  Cc: Peter Zijlstra, Brendan Gregg, Daniel Borkmann, Teng Qin, netdev,
	linux-kernel, kernel-team
In-Reply-To: <20170522224840.810121-1-ast@fb.com>

From: Teng Qin <qinteng@fb.com>

This commit adds sample to test attach BPF to HW_CACHE and RAW type
events into the trace_event sample. The test outputs a lot of things to
screen, therefore make sure it aborts on error so it's easier to see if
everything works. Also update clean-up logic to disable the perf event
before closing pmu_fd.

This commit also adds sample to test read HW_CACHE and RAW type event
counters from BPF program using bpf_perf_event_read. Refactored the
existing sample to fork individual task on each CPU, attach kprobe to
more controllable function, and more accurately check if each read on
every CPU returned with good value.

Signed-off-by: Teng Qin <qinteng@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 samples/bpf/bpf_helpers.h      |   2 +-
 samples/bpf/trace_event_user.c |  46 +++++++++++-
 samples/bpf/tracex6_kern.c     |  28 ++++++--
 samples/bpf/tracex6_user.c     | 155 +++++++++++++++++++++++++++++++----------
 4 files changed, 182 insertions(+), 49 deletions(-)

diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h
index 9a9c95f2c9fb..bbe1388ad149 100644
--- a/samples/bpf/bpf_helpers.h
+++ b/samples/bpf/bpf_helpers.h
@@ -31,7 +31,7 @@ static unsigned long long (*bpf_get_current_uid_gid)(void) =
 	(void *) BPF_FUNC_get_current_uid_gid;
 static int (*bpf_get_current_comm)(void *buf, int buf_size) =
 	(void *) BPF_FUNC_get_current_comm;
-static int (*bpf_perf_event_read)(void *map, int index) =
+static u64 (*bpf_perf_event_read)(void *map, u64 flags) =
 	(void *) BPF_FUNC_perf_event_read;
 static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
 	(void *) BPF_FUNC_clone_redirect;
diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index fa4336423da5..666761773fda 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c
@@ -122,13 +122,14 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 {
 	int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
 	int *pmu_fd = malloc(nr_cpus * sizeof(int));
-	int i;
+	int i, error = 0;
 
 	/* open perf_event on all cpus */
 	for (i = 0; i < nr_cpus; i++) {
 		pmu_fd[i] = sys_perf_event_open(attr, -1, i, -1, 0);
 		if (pmu_fd[i] < 0) {
 			printf("sys_perf_event_open failed\n");
+			error = 1;
 			goto all_cpu_err;
 		}
 		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
@@ -137,9 +138,13 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 	system("dd if=/dev/zero of=/dev/null count=5000k");
 	print_stacks();
 all_cpu_err:
-	for (i--; i >= 0; i--)
+	for (i--; i >= 0; i--) {
+		ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE, 0);
 		close(pmu_fd[i]);
+	}
 	free(pmu_fd);
+	if (error)
+		int_exit(0);
 }
 
 static void test_perf_event_task(struct perf_event_attr *attr)
@@ -150,7 +155,7 @@ static void test_perf_event_task(struct perf_event_attr *attr)
 	pmu_fd = sys_perf_event_open(attr, 0, -1, -1, 0);
 	if (pmu_fd < 0) {
 		printf("sys_perf_event_open failed\n");
-		return;
+		int_exit(0);
 	}
 	assert(ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
 	assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0) == 0);
@@ -175,11 +180,45 @@ static void test_bpf_perf_event(void)
 		.config = PERF_COUNT_SW_CPU_CLOCK,
 		.inherit = 1,
 	};
+	struct perf_event_attr attr_hw_cache_l1d = {
+		.sample_freq = SAMPLE_FREQ,
+		.freq = 1,
+		.type = PERF_TYPE_HW_CACHE,
+		.config =
+			PERF_COUNT_HW_CACHE_L1D |
+			(PERF_COUNT_HW_CACHE_OP_READ << 8) |
+			(PERF_COUNT_HW_CACHE_RESULT_ACCESS << 16),
+		.inherit = 1,
+	};
+	struct perf_event_attr attr_hw_cache_branch_miss = {
+		.sample_freq = SAMPLE_FREQ,
+		.freq = 1,
+		.type = PERF_TYPE_HW_CACHE,
+		.config =
+			PERF_COUNT_HW_CACHE_BPU |
+			(PERF_COUNT_HW_CACHE_OP_READ << 8) |
+			(PERF_COUNT_HW_CACHE_RESULT_MISS << 16),
+		.inherit = 1,
+	};
+	struct perf_event_attr attr_type_raw = {
+		.sample_freq = SAMPLE_FREQ,
+		.freq = 1,
+		.type = PERF_TYPE_RAW,
+		/* Intel Instruction Retired */
+		.config = 0xc0,
+		.inherit = 1,
+	};
 
 	test_perf_event_all_cpu(&attr_type_hw);
 	test_perf_event_task(&attr_type_hw);
 	test_perf_event_all_cpu(&attr_type_sw);
 	test_perf_event_task(&attr_type_sw);
+	test_perf_event_all_cpu(&attr_hw_cache_l1d);
+	test_perf_event_task(&attr_hw_cache_l1d);
+	test_perf_event_all_cpu(&attr_hw_cache_branch_miss);
+	test_perf_event_task(&attr_hw_cache_branch_miss);
+	test_perf_event_all_cpu(&attr_type_raw);
+	test_perf_event_task(&attr_type_raw);
 }
 
 
@@ -210,6 +249,7 @@ int main(int argc, char **argv)
 	}
 	test_bpf_perf_event();
 
+	printf("Success!\n");
 	int_exit(0);
 	return 0;
 }
diff --git a/samples/bpf/tracex6_kern.c b/samples/bpf/tracex6_kern.c
index be479c4af9e2..646f86426d09 100644
--- a/samples/bpf/tracex6_kern.c
+++ b/samples/bpf/tracex6_kern.c
@@ -3,22 +3,36 @@
 #include <uapi/linux/bpf.h>
 #include "bpf_helpers.h"
 
-struct bpf_map_def SEC("maps") my_map = {
+struct bpf_map_def SEC("maps") counters = {
 	.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
 	.key_size = sizeof(int),
 	.value_size = sizeof(u32),
-	.max_entries = 32,
+	.max_entries = 64,
+};
+struct bpf_map_def SEC("maps") values = {
+	.type = BPF_MAP_TYPE_HASH,
+	.key_size = sizeof(int),
+	.value_size = sizeof(u64),
+	.max_entries = 64,
 };
 
-SEC("kprobe/sys_write")
+SEC("kprobe/htab_map_get_next_key")
 int bpf_prog1(struct pt_regs *ctx)
 {
-	u64 count;
+	u64 count, *val;
+	s64 error;
 	u32 key = bpf_get_smp_processor_id();
-	char fmt[] = "CPU-%d   %llu\n";
 
-	count = bpf_perf_event_read(&my_map, key);
-	bpf_trace_printk(fmt, sizeof(fmt), key, count);
+	count = bpf_perf_event_read(&counters, key);
+	error = (s64)count;
+	if (error < 0 && error > -256)
+		return 0;
+
+	val = bpf_map_lookup_elem(&values, &key);
+	if (val)
+		*val = count;
+	else
+		bpf_map_update_elem(&values, &key, &count, BPF_NOEXIST);
 
 	return 0;
 }
diff --git a/samples/bpf/tracex6_user.c b/samples/bpf/tracex6_user.c
index ca7874ed77f4..3455ac458234 100644
--- a/samples/bpf/tracex6_user.c
+++ b/samples/bpf/tracex6_user.c
@@ -1,73 +1,152 @@
-#include <stdio.h>
-#include <unistd.h>
-#include <stdlib.h>
-#include <stdbool.h>
-#include <string.h>
+#define _GNU_SOURCE
+
+#include <assert.h>
 #include <fcntl.h>
-#include <poll.h>
-#include <sys/ioctl.h>
 #include <linux/perf_event.h>
 #include <linux/bpf.h>
-#include "libbpf.h"
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <sys/resource.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
 #include "bpf_load.h"
+#include "libbpf.h"
 #include "perf-sys.h"
 
 #define SAMPLE_PERIOD  0x7fffffffffffffffULL
 
-static void test_bpf_perf_event(void)
+static void check_on_cpu(int cpu, struct perf_event_attr *attr)
 {
-	int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
-	int *pmu_fd = malloc(nr_cpus * sizeof(int));
-	int status, i;
+	cpu_set_t set;
+	int pmu_fd;
+	__u64 value;
+	int error = 0;
+	/* Move to target CPU */
+	CPU_ZERO(&set);
+	CPU_SET(cpu, &set);
+	assert(sched_setaffinity(0, sizeof(set), &set) == 0);
+	/* Open perf event and attach to the perf_event_array */
+	pmu_fd = sys_perf_event_open(attr, -1/*pid*/, cpu/*cpu*/, -1/*group_fd*/, 0);
+	if (pmu_fd < 0) {
+		fprintf(stderr, "sys_perf_event_open failed on CPU %d\n", cpu);
+		error = 1;
+		goto on_exit;
+	}
+	assert(bpf_map_update_elem(map_fd[0], &cpu, &pmu_fd, BPF_ANY) == 0);
+	assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0) == 0);
+	/* Trigger the kprobe */
+	bpf_map_get_next_key(map_fd[1], &cpu, NULL);
+	/* Check the value */
+	if (bpf_map_lookup_elem(map_fd[1], &cpu, &value)) {
+		fprintf(stderr, "Value missing for CPU %d\n", cpu);
+		error = 1;
+		goto on_exit;
+	}
+	fprintf(stderr, "CPU %d: %llu\n", cpu, value);
+
+on_exit:
+	assert(bpf_map_delete_elem(map_fd[0], &cpu) == 0 || error);
+	assert(ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE, 0) == 0 || error);
+	assert(close(pmu_fd) == 0 || error);
+	assert(bpf_map_delete_elem(map_fd[1], &cpu) == 0 || error);
+	exit(error);
+}
 
-	struct perf_event_attr attr_insn_pmu = {
+static void test_perf_event_array(struct perf_event_attr *attr,
+				  const char *name)
+{
+	int i, status, nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+	pid_t pid[nr_cpus];
+
+	printf("Test reading %s counters\n", name);
+
+	for (i = 0; i < nr_cpus; i++) {
+		pid[i] = fork();
+		assert(pid[i] >= 0);
+		if (pid[i] == 0) {
+			check_on_cpu(i, attr);
+			exit(1);
+		}
+	}
+
+	for (i = 0; i < nr_cpus; i++) {
+		assert(waitpid(pid[i], &status, 0) == pid[i]);
+		assert(status == 0);
+	}
+}
+
+static void test_bpf_perf_event(void)
+{
+	struct perf_event_attr attr_cycles = {
 		.freq = 0,
 		.sample_period = SAMPLE_PERIOD,
 		.inherit = 0,
 		.type = PERF_TYPE_HARDWARE,
 		.read_format = 0,
 		.sample_type = 0,
-		.config = 0,/* PMU: cycles */
+		.config = PERF_COUNT_HW_CPU_CYCLES,
+	};
+	struct perf_event_attr attr_raw = {
+		.freq = 0,
+		.sample_period = SAMPLE_PERIOD,
+		.inherit = 0,
+		.type = PERF_TYPE_RAW,
+		.read_format = 0,
+		.sample_type = 0,
+		/* Intel Instruction Retired */
+		.config = 0xc0,
+	};
+	struct perf_event_attr attr_l1d_load = {
+		.freq = 0,
+		.sample_period = SAMPLE_PERIOD,
+		.inherit = 0,
+		.type = PERF_TYPE_HW_CACHE,
+		.read_format = 0,
+		.sample_type = 0,
+		.config =
+			PERF_COUNT_HW_CACHE_L1D |
+			(PERF_COUNT_HW_CACHE_OP_READ << 8) |
+			(PERF_COUNT_HW_CACHE_RESULT_ACCESS << 16),
+	};
+	struct perf_event_attr attr_llc_miss = {
+		.freq = 0,
+		.sample_period = SAMPLE_PERIOD,
+		.inherit = 0,
+		.type = PERF_TYPE_HW_CACHE,
+		.read_format = 0,
+		.sample_type = 0,
+		.config =
+			PERF_COUNT_HW_CACHE_LL |
+			(PERF_COUNT_HW_CACHE_OP_READ << 8) |
+			(PERF_COUNT_HW_CACHE_RESULT_MISS << 16),
 	};
 
-	for (i = 0; i < nr_cpus; i++) {
-		pmu_fd[i] = sys_perf_event_open(&attr_insn_pmu, -1/*pid*/, i/*cpu*/, -1/*group_fd*/, 0);
-		if (pmu_fd[i] < 0) {
-			printf("event syscall failed\n");
-			goto exit;
-		}
-
-		bpf_map_update_elem(map_fd[0], &i, &pmu_fd[i], BPF_ANY);
-		ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0);
-	}
-
-	status = system("ls > /dev/null");
-	if (status)
-		goto exit;
-	status = system("sleep 2");
-	if (status)
-		goto exit;
-
-exit:
-	for (i = 0; i < nr_cpus; i++)
-		close(pmu_fd[i]);
-	close(map_fd[0]);
-	free(pmu_fd);
+	test_perf_event_array(&attr_cycles, "HARDWARE-cycles");
+	test_perf_event_array(&attr_raw, "RAW-instruction-retired");
+	test_perf_event_array(&attr_l1d_load, "HW_CACHE-L1D-load");
+	test_perf_event_array(&attr_llc_miss, "HW_CACHE-LLC-miss");
 }
 
 int main(int argc, char **argv)
 {
+	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
 	char filename[256];
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 
+	setrlimit(RLIMIT_MEMLOCK, &r);
 	if (load_bpf_file(filename)) {
 		printf("%s", bpf_log_buf);
 		return 1;
 	}
 
 	test_bpf_perf_event();
-	read_trace_pipe();
 
+	printf("Success!\n");
 	return 0;
 }
-- 
2.9.3

^ permalink raw reply related

* [PATCH net-next 0/2] perf, bpf: add support for HW_CACHE and RAW events
From: Alexei Starovoitov @ 2017-05-22 22:48 UTC (permalink / raw)
  To: David S . Miller
  Cc: Peter Zijlstra, Brendan Gregg, Daniel Borkmann, Teng Qin, netdev,
	linux-kernel, kernel-team

Patch 1: add support for HW_CACHE and RAW perf events to bpf:
- similar to PERF_TYPE_RAW and PERF_TYPE_HARDWARE allow PERF_TYPE_HW_CACHE
  events to be accessed via bpf_perf_event_read()
- similar to PERF_TYPE_HARDWARE and PERF_TYPE_SOFTWARE allow bpf programs
  to attch to PERF_TYPE_HW_CACHE and PERF_TYPE_RAW events as
  BPF_PROG_TYPE_PERF_EVENT program type

Patch 2: add tests for HW_CACHE and RAW events

---
Peter,
please review patch 1. It looks trivial and as far as
we can see nothing else needed. The existing perf+bpf infra
covers it just fine.

Thanks!

Teng Qin (2):
  perf, bpf: add support for HW_CACHE and RAW events
  samples/bpf: add samples for HW_CACHE / RAW events

 kernel/bpf/arraymap.c          |   1 +
 kernel/events/core.c           |   4 +-
 kernel/trace/bpf_trace.c       |   1 +
 samples/bpf/bpf_helpers.h      |   2 +-
 samples/bpf/trace_event_user.c |  46 +++++++++++-
 samples/bpf/tracex6_kern.c     |  28 ++++++--
 samples/bpf/tracex6_user.c     | 155 +++++++++++++++++++++++++++++++----------
 7 files changed, 187 insertions(+), 50 deletions(-)

-- 
2.9.3

^ permalink raw reply

* Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions
From: Kees Cook @ 2017-05-22 23:07 UTC (permalink / raw)
  To: Djalal Harouni
  Cc: Solar Designer, linux-kernel, Network Development, LSM List,
	kernel-hardening@lists.openwall.com, Andy Lutomirski,
	Andrew Morton, Rusty Russell, Serge E. Hallyn, Jessica Yu,
	David S. Miller, James Morris, Paul Moore, Stephen Smalley,
	Greg Kroah-Hartman, Tetsuo Handa, Ingo Molnar, Linux API
In-Reply-To: <CAEiveUdb=yc5Gt_+pU_hfNdoNBP0JsUL1QUydNZgc2tD7n1h1w@mail.gmail.com>

On Mon, May 22, 2017 at 12:55 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Mon, May 22, 2017 at 6:43 PM, Solar Designer <solar@openwall.com> wrote:
>> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
>>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>>> >> disabled for all. Once set, this value can not be changed.
>>> >
>>> > What purpose does this securelevel-like property ("Once set, this value
>>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>>> > without this extra property, which is bypassable by e.g. explicitly
>>> > loaded kernel modules anyway (and that's OK).
>>>
>>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>>
>>> If you have some systems where modules are not updated for any given
>>> reason, then the only one who will be able to load a module is an
>>> administrator, basically this is a shortcut for:
>>>
>>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>>> auto-load 'netdev' modules.
>>>
>>> * Explicitly loading modules can be guarded by seccomp filters *per*
>>> app, so even if these apps have
>>>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
>>> has to remount some sysctl /proc/ entries read-only here and remove
>>> CAP_SYS_ADMIN for all apps anyway.
>>>
>>> This mainly serves the purpose of these systems that do not receive
>>> updates, if I don't want to expose those kernel interfaces what should
>>> I do ? then if I want to unload old versions and replace them with new
>>> ones what operation should be allowed ? and only real root of the
>>> system can do it. Hence, the "Once set, this value can not be changed"
>>> is more of a shortcut, also the idea was put in my mind based on how
>>> "modules_disabled" is disabled forever, and some other interfaces. I
>>> would say: it is easy to handle a transition from 1) "hey this system
>>> is still up to date, some features should be exposed" to 2) "this
>>> system is not up to date anymore, only root should expose some
>>> features..."
>>>
>>> Hmm, I am not sure if this answers your question ? :-)
>>
>> This answers my question, but in a way that I summarize as "there's no
>> good reason to include this securelevel-like property".
>>
>
> Hmm, sorry I did forget to add in my previous comment that with such
> systems, CAP_SYS_MODULE can be used to reset the
> "modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
> disable it privileged tasks can be triggered to overwrite the sysctl
> flag and get it back unless /proc is read-only... that's one of the
> points, it should not be so easy to relax it.

I'm on the fence. For modules_disabled and Yama, it was tied to
CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
not later be undone by an attacker gaining that privilege, keeping
them out of either kernel memory or existing user process memory.
Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
issue direct modprobe requests, but it's _possible_ via crazy symlink
games to trick capable processes into writing to sysctls. We've seen
this multiple times before, and it's a way for attackers to turn a
single privileged write into a privileged exec.

I might turn the question around, though: why would we want to have it
changeable at this setting?

I'm fine leaving that piece off, either way.

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply

* Re: [PATCH net-next 2/2] samples/bpf: add samples for HW_CACHE / RAW events
From: David Miller @ 2017-05-22 23:26 UTC (permalink / raw)
  To: ast; +Cc: peterz, bgregg, daniel, qinteng, netdev, linux-kernel,
	kernel-team
In-Reply-To: <20170522224840.810121-3-ast@fb.com>

From: Alexei Starovoitov <ast@fb.com>
Date: Mon, 22 May 2017 15:48:40 -0700

> @@ -31,7 +31,7 @@ static unsigned long long (*bpf_get_current_uid_gid)(void) =
>  	(void *) BPF_FUNC_get_current_uid_gid;
>  static int (*bpf_get_current_comm)(void *buf, int buf_size) =
>  	(void *) BPF_FUNC_get_current_comm;
> -static int (*bpf_perf_event_read)(void *map, int index) =
> +static u64 (*bpf_perf_event_read)(void *map, u64 flags) =
>  	(void *) BPF_FUNC_perf_event_read;

If the second argument really is "u64 flags", then please update
the comments in tools/include/uapi/linux/bpf.h as well.

Thank you.

^ permalink raw reply

* [PATCH v2 netdev] net: phy: marvell: Limit errata to 88m1101
From: Andrew Lunn @ 2017-05-22 23:28 UTC (permalink / raw)
  To: David Miller
  Cc: Daniel Walker, Harini Katakam, Florian Fainelli, netdev,
	Andrew Lunn

The 88m1101 has an errata when configuring autoneg. However, it was
being applied to many other Marvell PHYs as well. Limit its scope to
just the 88m1101.

Fixes: 76884679c644 ("phylib: Add support for Marvell 88e1111S and 88e1145")
Reported-by: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---

v2:
	Forgot netdev 

 drivers/net/phy/marvell.c | 67 ++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 88cd97b44ba6..77221d3f254f 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -267,35 +267,6 @@ static int marvell_config_aneg(struct phy_device *phydev)
 {
 	int err;
 
-	/* The Marvell PHY has an errata which requires
-	 * that certain registers get written in order
-	 * to restart autonegotiation
-	 */
-	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
-
-	if (err < 0)
-		return err;
-
-	err = phy_write(phydev, 0x1d, 0x1f);
-	if (err < 0)
-		return err;
-
-	err = phy_write(phydev, 0x1e, 0x200c);
-	if (err < 0)
-		return err;
-
-	err = phy_write(phydev, 0x1d, 0x5);
-	if (err < 0)
-		return err;
-
-	err = phy_write(phydev, 0x1e, 0);
-	if (err < 0)
-		return err;
-
-	err = phy_write(phydev, 0x1e, 0x100);
-	if (err < 0)
-		return err;
-
 	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
@@ -328,6 +299,42 @@ static int marvell_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+static int m88e1101_config_aneg(struct phy_device *phydev)
+{
+	int err;
+
+	/* This Marvell PHY has an errata which requires
+	 * that certain registers get written in order
+	 * to restart autonegotiation
+	 */
+	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
+
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, 0x1d, 0x1f);
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, 0x1e, 0x200c);
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, 0x1d, 0x5);
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, 0x1e, 0);
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, 0x1e, 0x100);
+	if (err < 0)
+		return err;
+
+	return marvell_config_aneg(phydev);
+}
+
 static int m88e1111_config_aneg(struct phy_device *phydev)
 {
 	int err;
@@ -1960,7 +1967,7 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.probe = marvell_probe,
 		.config_init = &marvell_config_init,
-		.config_aneg = &marvell_config_aneg,
+		.config_aneg = &m88e1101_config_aneg,
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH net-next 00/20] net: dsa: distribute switch events
From: David Miller @ 2017-05-22 23:31 UTC (permalink / raw)
  To: f.fainelli; +Cc: vivien.didelot, netdev, linux-kernel, kernel, andrew
In-Reply-To: <ce634f83-35b0-abbf-a18b-61d3feccc097@gmail.com>

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 22 May 2017 13:01:06 -0700

> For this entire series:
> 
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> on a 7445 (bcm-sf2), normal bridging still worked, and bridging with
> VLAN filtering also did, just like adding VLANs to user-facing ports
> also did.
> 
> Great job!

Series applied, thanks.

Andrew, if you have any feedback or have any reservations about what
this series is doing, don't worry.  I'm sure you can work it out with
Vivien and he'll make whatever changes are necessary to make you happy
about it.

Thanks.

^ permalink raw reply

* Re: [PATCH net-next 2/2] samples/bpf: add samples for HW_CACHE / RAW events
From: Alexei Starovoitov @ 2017-05-22 23:35 UTC (permalink / raw)
  To: David Miller
  Cc: peterz, bgregg, daniel, qinteng, netdev, linux-kernel,
	kernel-team
In-Reply-To: <20170522.192651.252662920166760334.davem@davemloft.net>

On 5/22/17 4:26 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@fb.com>
> Date: Mon, 22 May 2017 15:48:40 -0700
>
>> @@ -31,7 +31,7 @@ static unsigned long long (*bpf_get_current_uid_gid)(void) =
>>  	(void *) BPF_FUNC_get_current_uid_gid;
>>  static int (*bpf_get_current_comm)(void *buf, int buf_size) =
>>  	(void *) BPF_FUNC_get_current_comm;
>> -static int (*bpf_perf_event_read)(void *map, int index) =
>> +static u64 (*bpf_perf_event_read)(void *map, u64 flags) =
>>  	(void *) BPF_FUNC_perf_event_read;
>
> If the second argument really is "u64 flags", then please update
> the comments in tools/include/uapi/linux/bpf.h as well.

of course.
As independent patch, I assume.
For both tools/include/uapi/...bpf.h and include/uapi/...bpf.h

It has some info:
/* BPF_FUNC_perf_event_output and BPF_FUNC_perf_event_read flags. */
#define BPF_F_INDEX_MASK                0xffffffffULL
#define BPF_F_CURRENT_CPU               BPF_F_INDEX_MASK

yet perf_event_read() is least documented. hmm.
  * u64 bpf_perf_event_read(&map, index)
  *     Return: Number events read or error code

that needs to be fixed.

^ permalink raw reply

* Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions
From: Andy Lutomirski @ 2017-05-22 23:38 UTC (permalink / raw)
  To: Kees Cook
  Cc: Djalal Harouni, Solar Designer, linux-kernel, Network Development,
	LSM List, kernel-hardening@lists.openwall.com, Andy Lutomirski,
	Andrew Morton, Rusty Russell, Serge E. Hallyn, Jessica Yu,
	David S. Miller, James Morris, Paul Moore, Stephen Smalley,
	Greg Kroah-Hartman, Tetsuo Handa, Ingo Molnar
In-Reply-To: <CAGXu5jKGnG74KE-k9JPaH1bNqT5nbVioaeu_5sAKQ+4kgp-0Ng@mail.gmail.com>

On Mon, May 22, 2017 at 4:07 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, May 22, 2017 at 12:55 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> On Mon, May 22, 2017 at 6:43 PM, Solar Designer <solar@openwall.com> wrote:
>>> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>>>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
>>>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>>>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>>>> >> disabled for all. Once set, this value can not be changed.
>>>> >
>>>> > What purpose does this securelevel-like property ("Once set, this value
>>>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>>>> > without this extra property, which is bypassable by e.g. explicitly
>>>> > loaded kernel modules anyway (and that's OK).
>>>>
>>>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>>>
>>>> If you have some systems where modules are not updated for any given
>>>> reason, then the only one who will be able to load a module is an
>>>> administrator, basically this is a shortcut for:
>>>>
>>>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>>>> auto-load 'netdev' modules.
>>>>
>>>> * Explicitly loading modules can be guarded by seccomp filters *per*
>>>> app, so even if these apps have
>>>>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
>>>> has to remount some sysctl /proc/ entries read-only here and remove
>>>> CAP_SYS_ADMIN for all apps anyway.
>>>>
>>>> This mainly serves the purpose of these systems that do not receive
>>>> updates, if I don't want to expose those kernel interfaces what should
>>>> I do ? then if I want to unload old versions and replace them with new
>>>> ones what operation should be allowed ? and only real root of the
>>>> system can do it. Hence, the "Once set, this value can not be changed"
>>>> is more of a shortcut, also the idea was put in my mind based on how
>>>> "modules_disabled" is disabled forever, and some other interfaces. I
>>>> would say: it is easy to handle a transition from 1) "hey this system
>>>> is still up to date, some features should be exposed" to 2) "this
>>>> system is not up to date anymore, only root should expose some
>>>> features..."
>>>>
>>>> Hmm, I am not sure if this answers your question ? :-)
>>>
>>> This answers my question, but in a way that I summarize as "there's no
>>> good reason to include this securelevel-like property".
>>>
>>
>> Hmm, sorry I did forget to add in my previous comment that with such
>> systems, CAP_SYS_MODULE can be used to reset the
>> "modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
>> disable it privileged tasks can be triggered to overwrite the sysctl
>> flag and get it back unless /proc is read-only... that's one of the
>> points, it should not be so easy to relax it.
>
> I'm on the fence. For modules_disabled and Yama, it was tied to
> CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
> not later be undone by an attacker gaining that privilege, keeping
> them out of either kernel memory or existing user process memory.
> Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
> a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
> issue direct modprobe requests, but it's _possible_ via crazy symlink
> games to trick capable processes into writing to sysctls. We've seen
> this multiple times before, and it's a way for attackers to turn a
> single privileged write into a privileged exec.
>
> I might turn the question around, though: why would we want to have it
> changeable at this setting?
>
> I'm fine leaving that piece off, either way.

I think that having the un-resettable mode is unnecessary.  We should
have option that disables loading modules entirely and cannot be
unset.  (That means no explicit loads and not implicit loads.)  Maybe
we already have this.  Otherwise, tightening caps needed for implicit
loads should just be a normal yes/no setting IMO.

^ permalink raw reply

* Re: [PATCH net-next 2/2] samples/bpf: add samples for HW_CACHE / RAW events
From: David Miller @ 2017-05-22 23:40 UTC (permalink / raw)
  To: ast; +Cc: peterz, bgregg, daniel, qinteng, netdev, linux-kernel,
	kernel-team
In-Reply-To: <63c0ec55-9cb7-50a0-2020-fe17283e532b@fb.com>

From: Alexei Starovoitov <ast@fb.com>
Date: Mon, 22 May 2017 16:35:07 -0700

> yet perf_event_read() is least documented. hmm.
>  * u64 bpf_perf_event_read(&map, index)
>  *     Return: Number events read or error code
> 
> that needs to be fixed.

That's exactly what I was talking about :-)

^ permalink raw reply

* Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions
From: Kees Cook @ 2017-05-22 23:52 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Djalal Harouni, Solar Designer, linux-kernel, Network Development,
	LSM List, kernel-hardening@lists.openwall.com, Andrew Morton,
	Rusty Russell, Serge E. Hallyn, Jessica Yu, David S. Miller,
	James Morris, Paul Moore, Stephen Smalley, Greg Kroah-Hartman,
	Tetsuo Handa, Ingo Molnar, Linux API
In-Reply-To: <CALCETrXyf3vM+NdFNcjwaOZs1KFJomuTL0DobXoH9Z68k8dknQ@mail.gmail.com>

On Mon, May 22, 2017 at 4:38 PM, Andy Lutomirski <luto@kernel.org> wrote:
> I think that having the un-resettable mode is unnecessary.  We should
> have option that disables loading modules entirely and cannot be
> unset.  (That means no explicit loads and not implicit loads.)  Maybe
> we already have this.  Otherwise, tightening caps needed for implicit
> loads should just be a normal yes/no setting IMO.

Yup, /proc/sys/kernel/modules_disabled already does this.

-- 
Kees Cook
Pixel Security

^ permalink raw reply

* Re: [PATCH 00/12] Netfilter/IPVS fixes for net
From: David Miller @ 2017-05-22 23:54 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <20170521222538.GA2370@salvia>

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 22 May 2017 00:25:38 +0200

> Could you merge net into net-next as well? I have several patches for
> net-next that need to apply on these fixes. No rush BTW.

Sure, no problem.

As soon as Linus takes in my pull request from today, I will do this
and let you know.

^ permalink raw reply

* Re: [patch net-next RFC] net: sched: cls_api: make reclassify return all the way back to the original tp
From: Cong Wang @ 2017-05-22 23:57 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
	David Ahern, Eric Dumazet, Stephen Hemminger, Daniel Borkmann,
	Alexander Duyck, Simon Horman, mlxsw
In-Reply-To: <20170522150942.1703-1-jiri@resnulli.us>

On Mon, May 22, 2017 at 8:09 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> With the introduction of chain goto action, the reclassification would
> cause the re-iteration of the actual chain. But it perhaps makes more
> sense to restart the whole thing. Thoughts?

I think reclassification is meant to restart the whole logic rather
than just one chain. So your patch makes sense to me, but not
sure if there is any corner case I miss.

^ permalink raw reply

* Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.
From: David Miller @ 2017-05-22 23:59 UTC (permalink / raw)
  To: vyasevich; +Cc: netdev, vyasevic, makita.toshiaki
In-Reply-To: <1495114265-23368-2-git-send-email-vyasevic@redhat.com>

From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Thu, 18 May 2017 09:31:03 -0400

> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

This is a tough one.  I can certainly sympathize with your frustration
trying to track this down.

Clearing NETIF_F_HW_CSUM completely is the most conservative change.

However, for all the (perhaps many) cards upon which the checksumming
does work properly in Q-in-Q situations, this change could be
introducing non-trivial performance regressions.

So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
on balance, the best way forward.

Thanks.

^ permalink raw reply

* [PATCH net-next] bpf: update perf event helper function signature and documentation
From: Teng Qin @ 2017-05-23  0:39 UTC (permalink / raw)
  To: David S . Miller
  Cc: Peter Zijlstra, Brendan Gregg, Daniel Borkmann,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Kernel Team,
	Alexei Starovoitov

From: Teng Qin <qinteng@fb.com>

This commit updates function signature of the bpf_perf_event_output and
bpf_perf_event_read helpers to match their implementation. Also updates
their documentation in the header files.

Signed-off-by: Teng Qin <qinteng@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 include/uapi/linux/bpf.h       | 11 +++++++----
 samples/bpf/bpf_helpers.h      |  5 ++---
 tools/include/uapi/linux/bpf.h | 11 +++++++----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 94dfa9d..e78aece 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -313,8 +313,11 @@ union bpf_attr {
  *     @flags: room for future extensions
  *     Return: 0 on success or negative error
  *
- * u64 bpf_perf_event_read(&map, index)
- *     Return: Number events read or error code
+ * u64 bpf_perf_event_read(map, flags)
+ *     read perf event counter value
+ *     @map: pointer to perf_event_array map
+ *     @flags: index of event in the map or bitmask flags
+ *     Return: value of perf event counter read or error code
  *
  * int bpf_redirect(ifindex, flags)
  *     redirect to another netdev
@@ -328,11 +331,11 @@ union bpf_attr {
  *     @skb: pointer to skb
  *     Return: realm if != 0
  *
- * int bpf_perf_event_output(ctx, map, index, data, size)
+ * int bpf_perf_event_output(ctx, map, flags, data, size)
  *     output perf raw sample
  *     @ctx: struct pt_regs*
  *     @map: pointer to perf_event_array map
- *     @index: index of event in the map
+ *     @flags: index of event in the map or bitmask flags
  *     @data: data on stack to be output as raw data
  *     @size: size of data
  *     Return: 0 on success or negative error
diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h
index 9a9c95f..a94ce42 100644
--- a/samples/bpf/bpf_helpers.h
+++ b/samples/bpf/bpf_helpers.h
@@ -37,9 +37,8 @@ static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
 	(void *) BPF_FUNC_clone_redirect;
 static int (*bpf_redirect)(int ifindex, int flags) =
 	(void *) BPF_FUNC_redirect;
-static int (*bpf_perf_event_output)(void *ctx, void *map,
-				    unsigned long long flags, void *data,
-				    int size) =
+static int (*bpf_perf_event_output)(void *ctx, void *map, u64 flags,
+	                            void *data, int size) =
 	(void *) BPF_FUNC_perf_event_output;
 static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
 	(void *) BPF_FUNC_get_stackid;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 94dfa9d..e78aece 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -313,8 +313,11 @@ union bpf_attr {
  *     @flags: room for future extensions
  *     Return: 0 on success or negative error
  *
- * u64 bpf_perf_event_read(&map, index)
- *     Return: Number events read or error code
+ * u64 bpf_perf_event_read(map, flags)
+ *     read perf event counter value
+ *     @map: pointer to perf_event_array map
+ *     @flags: index of event in the map or bitmask flags
+ *     Return: value of perf event counter read or error code
  *
  * int bpf_redirect(ifindex, flags)
  *     redirect to another netdev
@@ -328,11 +331,11 @@ union bpf_attr {
  *     @skb: pointer to skb
  *     Return: realm if != 0
  *
- * int bpf_perf_event_output(ctx, map, index, data, size)
+ * int bpf_perf_event_output(ctx, map, flags, data, size)
  *     output perf raw sample
  *     @ctx: struct pt_regs*
  *     @map: pointer to perf_event_array map
- *     @index: index of event in the map
+ *     @flags: index of event in the map or bitmask flags
  *     @data: data on stack to be output as raw data
  *     @size: size of data
  *     Return: 0 on success or negative error
-- 
2.9.3




^ permalink raw reply related

* [PATCH net-next] net: rfs: Don't reset RFS entries when nothing changed
From: gfree.wind @ 2017-05-23  0:45 UTC (permalink / raw)
  To: davem, netdev; +Cc: Gao Feng

From: Gao Feng <gfree.wind@vip.163.com>

When the new RFS table size specified by sysctl equals the old one,
there is nothing changed actually. So it is unnecessary to reset the
RFS table entris.

Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
---
 net/core/sysctl_net_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index ea23254..80b6a7e 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -69,11 +69,12 @@ static int rps_sock_flow_sysctl(struct ctl_table *table, int write,
 				}
 				rps_cpu_mask = roundup_pow_of_two(nr_cpu_ids) - 1;
 				sock_table->mask = size - 1;
+
+				for (i = 0; i < size; i++)
+					sock_table->ents[i] = RPS_NO_CPU;
 			} else
 				sock_table = orig_sock_table;
 
-			for (i = 0; i < size; i++)
-				sock_table->ents[i] = RPS_NO_CPU;
 		} else
 			sock_table = NULL;
 
-- 
1.9.1

^ permalink raw reply related

* Re: [PATCH] net: ieee802154: fix net_device reference release too early
From: zhanglin496 @ 2017-05-23  1:45 UTC (permalink / raw)
  To: stefan; +Cc: aar, davem, linux-wpan, netdev, linux-kernel

Hello.

Sorry too late to reply.
>
> Hello.
>
> On Thu, 2017-05-18 at 15:14, Stefan Schmidt wrote:
> > Hello.
> >
> > On Thu, 2017-05-18 at 15:50, linzhang wrote:
> > > This patch fixes the kernel oops when release net_device reference in
> > > advance. In function raw_sendmsg(i think the dgram_sendmsg has the same
> > > problem), there is a race condition between dev_put and dev_queue_xmit
> > > when the device is gong that maybe lead to dev_queue_ximt to see
> > > an illegal net_device pointer.
> > >
> >
> > You have a test case to reproduce this oops? I fear I have not seen
> > one.
>
> If you have a test case handy adding it to the commit would be handy. If you do
> not have one around we can do without.
>

My test kernel is 3.13.0-32.
Becasue i am not have a real 802154 device, so i change lowpan_newlink
function to this:

        /* find and hold real wpan device */
        real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
        if (!real_dev)
                return -ENODEV;
//      if (real_dev->type != ARPHRD_IEEE802154) {
//              dev_put(real_dev);
//              return -EINVAL;
//      }
        lowpan_dev_info(dev)->real_dev = real_dev;
        lowpan_dev_info(dev)->fragment_tag = 0;
        mutex_init(&lowpan_dev_info(dev)->dev_list_mtx);

Also, in order to simulate preempt, i change the raw_sendmsg function to this:

        skb->dev = dev;
        skb->sk  = sk;
        skb->protocol = htons(ETH_P_IEEE802154);
        dev_put(dev);
        //simulate preempt
        schedule_timeout_uninterruptible(30 * HZ);
        err = dev_queue_xmit(skb);
        if (err > 0)
                err = net_xmit_errno(err);

and this is my userspace test code named test_send_data:

#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>
int main(int argc, char **argv)
{
        char buf[127];
        int sockfd;
        sockfd = socket(AF_IEEE802154, SOCK_RAW, 0);
        if (sockfd < 0) {
                printf("create sockfd error: %s\n", strerror(errno));
                return -1;
        }
        send(sockfd, buf, sizeof(buf), 0);
        return 0;
}

This is my test case:
root@zhanglin-x-computer:~/develop/802154# uname -a
Linux zhanglin-x-computer 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15
03:51:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
root@zhanglin-x-computer:~/develop/802154# ip link add link eth0 name
lowpan0 type lowpan
root@zhanglin-x-computer:~/develop/802154#
//keep the lowpan0 device down
root@zhanglin-x-computer:~/develop/802154# ./test_send_data &
//wait a while
root@zhanglin-x-computer:~/develop/802154# ip link del link dev lowpan0
//the device is gone
//oops
[381.303307] general protection fault: 0000 [#1]SMP
[381.303407] Modules linked in: af_802154 6lowpan bnep rfcomm
bluetooth nls_iso8859_1 snd_hda_codec_hdmi snd_hda_codec_realtek
rts5139(C) snd_hda_intel
snd_had_codec snd_hwdep snd_pcm snd_page_alloc snd_seq_midi
snd_seq_midi_event snd_rawmidi snd_req intel_rapl snd_seq_device
coretemp i915 kvm_intel
kvm snd_timer snd crct10dif_pclmul crc32_pclmul ghash_clmulni_intel
cypted drm_kms_helper drm i2c_algo_bit soundcore video mac_hid
parport_pc ppdev ip parport hid_generic
usbhid hid ahci r8169 mii libahdi
[381.304286] CPU:1 PID: 2524 Commm: 1 Tainted: G C 0 3.13.0-32-generic
#57-Ubuntu
[381.304409] Hardware name: Haier Haier DT Computer/Haier DT Codputer,
BIOS FIBT19H02_X64 06/09/2014
[381.304546] tasks: ffff000096965fc0 ti: ffffB0013779c000 task.ti:
ffffB8013779c000
[381.304659] RIP: 0010:[<ffffffff01621fe1>] [<ffffffff81621fe1>]
__dev_queue_ximt+0x61/0x500
[381.304798] RSP: 0018:ffffB8013779dca0 EFLAGS: 00010202
[381.304880] RAX: 272b031d57565351 RBX: 0000000000000000 RCX: ffff8800968f1a00
[381.304987] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8800968f1a00
[381.305095] RBP: ffff8e013773dce0 R08: 0000000000000266 R09: 0000000000000004
[381.305202] R10: 0000000000000004 R11: 0000000000000005 R12: ffff88013902e000
[381.305310] R13: 000000000000007f R14: 000000000000007f R15: ffff8800968f1a00
[381.305418] FS:  00007fc57f50f740(0000) GS: ffff88013fc80000(0000)
knlGS: 0000000000000000
[381.305540] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[381.305627] CR2: 00007fad0841c000 CR3: 00000001368dd000 CR4: 00000000001007e0
[361.905734] Stack:
[381.305768]  00000000002052d0 000000003facb30a ffff88013779dcc0
ffff880137764000
[381.305898]  ffff88013779de70 000000000000007f 000000000000007f
ffff88013902e000
[381.306026]  ffff88013779dcf0 ffffffff81622490 ffff88013779dd39
ffffffffa03af9f1
[381.306155] Call Trace:
[381.306202]  [<ffffffff81622490>] dev_queue_xmit+0x10/0x20
[381.306294]  [<ffffffffa03af9f1>] raw_sendmsg+0x1b1/0x270 [af_802154]
[381.306396]  [<ffffffffa03af054>] ieee802154_sock_sendmsg+0x14/0x20 [af_802154]
[381.306512]  [<ffffffff816079eb>] sock_sendmsg+0x8b/0xc0
[381.306600]  [<ffffffff811d52a5>] ? __d_alloc+0x25/0x180
[381.306687]  [<ffffffff811a1f56>] ? kmem_cache_alloc_trace+0x1c6/0x1f0
[381.306791]  [<ffffffff81607b91>] SYSC_sendto+0x121/0x1c0
[381.306878]  [<ffffffff8109ddf4>] ? vtime_account_user+x54/0x60
[381.306975]  [<ffffffff81020d45>] ? syscall_trace_enter+0x145/0x250
[381.307073]  [<ffffffff816086ae>] SyS_sendto+0xe/0x10
[381.307156]  [<ffffffff8172c87f>] tracesys+0xe1/0xe6
[381.307233] Code: c6 a1 a4 ff 41 8b 57 78 49 8b 47 20 85 d2 48 8b 80
78 07 00 00 75 21 49 8b 57 18 48 85 d2 74 18 48 85 c0 74 13 8b 92 ac
01 00 00 <3b> 50 10 73 08 8b 44 90 14 41 89 47 78 41 f6 84 24 d5 00 00
00
[381.307801] RIP [<ffffffff81621fe1>] _dev_queue_xmit+0x61/0x500
[381.307901]  RSP <ffff88013779dca0>
[381.347512] Kernel panic - not syncing: Fatal exception in interrupt
[381.347747] drm_kms_helper: panic occurred, switching back to text console


> > > So i think that dev_put should be behind of the dev_queue_xmit.
> > >
> > > Also, explicit set skb->sk is needless, sock_alloc_send_skb is
> > > already set it.
> >
> > You could have put this fixup in a different patch.
>

Thanks, i will repost it.

> I actually would request you to split this into two patches. One for the
> removal of the sk setting and one for the race condition fix.
>
> > > Signed-off-by: linzhang <xiaolou4617@gmail.com>
> >
> > This looks more like a username instead of a real name. If you have Lin
> > Zhang as you English real name that would be better here. :)
>
> This would be also appreciated.

Yes, my real name is Lin Zhang, sorry to make you confusion.

> > > ---
> > >  net/ieee802154/socket.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> > > index eedba76..a60658c 100644
> > > --- a/net/ieee802154/socket.c
> > > +++ b/net/ieee802154/socket.c
> > > @@ -301,15 +301,14 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >             goto out_skb;
> > >
> > >     skb->dev = dev;
> > > -   skb->sk  = sk;
> > >     skb->protocol = htons(ETH_P_IEEE802154);
> > >
> > > -   dev_put(dev);
> > > -
> > >     err = dev_queue_xmit(skb);
> > >     if (err > 0)
> > >             err = net_xmit_errno(err);
> > >
> > > +   dev_put(dev);
> > > +
> > >     return err ?: size;
> > >
> > >  out_skb:
> > > @@ -690,15 +689,14 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >             goto out_skb;
> > >
> > >     skb->dev = dev;
> > > -   skb->sk  = sk;
> > >     skb->protocol = htons(ETH_P_IEEE802154);
> > >
> > > -   dev_put(dev);
> > > -
> > >     err = dev_queue_xmit(skb);
> > >     if (err > 0)
> > >             err = net_xmit_errno(err);
> > >
> > > +   dev_put(dev);
> > > +
> > >     return err ?: size;
> >
> > Going to give this a test ride here now.
>
> I gave it a ride in my testbed and I encountered no problems. While I have never
> seen the race and oops myself doing the dev_put before the xmit can surely lead to
> such a race and the fix is valid.
>
> Once you have done the changes requested above and re-submit your two patches you can
> add my
>
> Acked-by: Stefan Schmidt <stefan@osg.samsung.com>
>
> to both of them.
>
> regards
> Stefan Schmidt

In my opinion, there is always exist a chance that the device is gong
before call dev_queue_xmit.

I think the latest kernel is have the same problem. If you have a real 802154 device, 
maybe use the test case as above, thanks.

Please forgive me for my weak English.
Thanks for reviewing!
regards

^ permalink raw reply

* RE: [PATCH] net: fec: add post PHY reset delay DT property
From: Andy Duan @ 2017-05-23  2:04 UTC (permalink / raw)
  To: Quentin Schulz, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	mark.rutland-5wv7dgnIgG8@public.gmane.org
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org
In-Reply-To: <20170522091517.6857-1-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

From: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Sent: Monday, May 22, 2017 5:15 PM
>Some PHY require to wait for a bit after the reset GPIO has been toggled. This
>adds support for the DT property `phy-reset-post-delay` which gives the delay
>in milliseconds to wait after reset.
>
>If the DT property is not given, no delay is observed. Post reset delay greater
>than 1000ms are invalid and are default to 1ms.
>
>Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>---
> Documentation/devicetree/bindings/net/fsl-fec.txt |  5 +++++
> drivers/net/ethernet/freescale/fec_main.c         | 17 +++++++++++++++--
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>b/Documentation/devicetree/bindings/net/fsl-fec.txt
>index a1e3693cca16..8795e8ca5793 100644
>--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>@@ -15,6 +15,11 @@ Optional properties:
> - phy-reset-active-high : If present then the reset sequence using the GPIO
>   specified in the "phy-reset-gpios" property is reversed (H=reset state,
>   L=operation state).
>+- phy-reset-post-delay : Post reset delay in milliseconds. If present
>+then
>+  a delay of phy-reset-post-delay milliseconds will be observed after
>+the
>+  phy-reset-gpios has been toggled. Can be omitted thus no delay is
>+  observed. Delay is in range of 1ms to 1000ms. If given delay is
>+higher
>+  than 1000ms, 1ms delay is done instead.
> - phy-supply : regulator that powers the Ethernet PHY.
> - phy-handle : phandle to the PHY device connected to this device.
> - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 56a563f90b0b..00a7fd0bcd59 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -3192,7 +3192,7 @@ static int fec_reset_phy(struct platform_device
>*pdev)  {
> 	int err, phy_reset;
> 	bool active_high = false;
>-	int msec = 1;
>+	int msec = 1, phy_post_delay = 0;
> 	struct device_node *np = pdev->dev.of_node;
>
> 	if (!np)
>@@ -3210,7 +3210,6 @@ static int fec_reset_phy(struct platform_device
>*pdev)
> 		return 0;
>
> 	active_high = of_property_read_bool(np, "phy-reset-active-high");
>-

No necessary change here.

> 	err = devm_gpio_request_one(&pdev->dev, phy_reset,
> 			active_high ? GPIOF_OUT_INIT_HIGH :
>GPIOF_OUT_INIT_LOW,
> 			"phy-reset");
>@@ -3219,6 +3218,11 @@ static int fec_reset_phy(struct platform_device
>*pdev)
> 		return err;
> 	}
>
>+	err = of_property_read_u32(np, "phy-reset-post-delay",
>&phy_post_delay);
>+	/* valid reset duration should be less than 1s */
>+	if (!err && phy_post_delay > 1000)
>+		phy_post_delay = 1;
>+
Put the dt parse before  . devm_gpio_request_one() that seems better.
Others are fine.


> 	if (msec > 20)
> 		msleep(msec);
> 	else
>@@ -3226,6 +3230,15 @@ static int fec_reset_phy(struct platform_device
>*pdev)
>
> 	gpio_set_value_cansleep(phy_reset, !active_high);
>
>+	if (!phy_post_delay)
>+		return 0;
>+
>+	if (phy_post_delay > 20)
>+		msleep(phy_post_delay);
>+	else
>+		usleep_range(phy_post_delay * 1000,
>+			     phy_post_delay * 1000 + 1000);
>+
> 	return 0;
> }
> #else /* CONFIG_OF */
>--
>2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: arch: arm: bpf: Converting cBPF to eBPF for arm 32 bit
From: Shubham Bansal @ 2017-05-23  2:58 UTC (permalink / raw)
  To: Kees Cook
  Cc: Daniel Borkmann, David Miller, Mircea Gherzan,
	Network Development, kernel-hardening@lists.openwall.com,
	linux-arm-kernel@lists.infradead.org, ast
In-Reply-To: <CAGXu5jLYunVCJGCfHPebKDaoQ71hdMGq4HhdDxTYpBQw_HXUYQ@mail.gmail.com>

Hi,

On testing the eBPF JIT with CONFIG_FRAME_POINTER I got the following
crash for non jitted testcase.

[   72.032494] test_bpf: #267 BPF_MAXINSNS: Call heavy transformations
jited:0 1112799
[   92.304815] NMI watchdog: BUG: soft lockup - CPU#0 stuck for 22s!
[insmod:104]
[   92.305050] Modules linked in: test_bpf(+)
[   92.305516] CPU: 0 PID: 104 Comm: insmod Not tainted
4.11.0-10603-g13e0988-dirty #21
[   92.305630] Hardware name: ARM-Versatile Express
[   92.305943] task: c75d5280 task.stack: c61b8000
[   92.306383] PC is at __bpf_prog_run+0x818/0x17a8
[   92.306449] LR is at __bpf_prog_run+0xab8/0x17a8
[   92.306510] pc : [<c0407c08>]    lr : [<c0407ea8>]    psr: 20000013
[   92.306510] sp : c61b9a88  ip : c61b9a88  fp : c61b9d4c
[   92.306629] r10: c0404104  r9 : 00000000  r8 : 00000000
[   92.306744] r7 : c0e0b500  r6 : c0c39bb0  r5 : c61b9ad0  r4 : ca314840
[   92.306882] r3 : c0e0b7fc  r2 : 00000000  r1 : c61b9ad8  r0 : 00000000
[   92.307070] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   92.307285] Control: 10c5387d  Table: 661e0059  DAC: 00000051
[   92.307503] CPU: 0 PID: 104 Comm: insmod Not tainted
4.11.0-10603-g13e0988-dirty #21
[   92.307575] Hardware name: ARM-Versatile Express
[   92.307651] Backtrace:
[   92.307868] [<c030caec>] (dump_backtrace) from [<c030cda8>]
(show_stack+0x18/0x1c)
[   92.308003]  r7:c1503db8 r6:60000193 r5:00000000 r4:c1570f30
[   92.308085] [<c030cd90>] (show_stack) from [<c064b198>]
(dump_stack+0x90/0xa4)
[   92.308195] [<c064b108>] (dump_stack) from [<c030900c>] (show_regs+0x14/0x18)
[   92.308281]  r7:c1503db8 r6:c14488b8 r5:c16aaea0 r4:c61b8000
[   92.308346] [<c0308ff8>] (show_regs) from [<c03df2a4>]
(watchdog_timer_fn+0x24c/0x2c4)
[   92.308423] [<c03df058>] (watchdog_timer_fn) from [<c03b70d8>]
(__hrtimer_run_queues+0x180/0x318)
[   92.308514]  r10:c03df058 r9:00000003 r8:c1503cbc r7:c7ead580
r6:c7ead5c0 r5:c61b8000
[   92.308578]  r4:c7ead8d8
[   92.308635] [<c03b6f58>] (__hrtimer_run_queues) from [<c03b74e8>]
(hrtimer_interrupt+0xb4/0x204)
[   92.308728]  r10:7fffffff r9:00000003 r8:c7ead5f8 r7:c7ead618
r6:c7ead638 r5:c1448580
[   92.308789]  r4:c7ead580
[   92.308835] [<c03b7434>] (hrtimer_interrupt) from [<c03113fc>]
(twd_handler+0x38/0x48)
[   92.308914]  r10:c0404104 r9:00000010 r8:c1504330 r7:00000001
r6:c701e900 r5:00000000
[   92.308974]  r4:00000001
[   92.309021] [<c03113c4>] (twd_handler) from [<c03a1238>]
(handle_percpu_devid_irq+0x90/0x244)
[   92.309091]  r5:00000000 r4:c7020540
[   92.309165] [<c03a11a8>] (handle_percpu_devid_irq) from
[<c039c148>] (generic_handle_irq+0x2c/0x3c)
[   92.309254]  r10:c0404104 r9:c8803100 r8:c7004a00 r7:00000001
r6:00000000 r5:00000000
[   92.309319]  r4:c1449ed0 r3:c03a11a8
[   92.309369] [<c039c11c>] (generic_handle_irq) from [<c039c6f0>]
(__handle_domain_irq+0x64/0xbc)
[   92.309445] [<c039c68c>] (__handle_domain_irq) from [<c0301808>]
(gic_handle_irq+0x5c/0xa0)
[   92.309525]  r9:c8803100 r8:c8802100 r7:c61b9a38 r6:c880210c
r5:c1571848 r4:c1504330
[   92.309596] [<c03017ac>] (gic_handle_irq) from [<c030d98c>]
(__irq_svc+0x6c/0x90)
[   92.309731] Exception stack(0xc61b9a38 to 0xc61b9a80)
[   92.309943] 9a20:
    00000000 c61b9ad8
[   92.310184] 9a40: 00000000 c0e0b7fc ca314840 c61b9ad0 c0c39bb0
c0e0b500 00000000 00000000
[   92.310377] 9a60: c0404104 c61b9d4c c61b9a88 c61b9a88 c0407ea8
c0407c08 20000013 ffffffff
[   92.310595]  r9:c61b8000 r8:00000000 r7:c61b9a6c r6:ffffffff
r5:20000013 r4:c0407c08
[   92.311103] [<c04073f0>] (__bpf_prog_run) from [<bf15759c>]
(test_bpf_init+0x59c/0x1000 [test_bpf])
[   92.311262]  r10:bf123094 r9:ca2fa020 r8:00000000 r7:bf123128
r6:53edefe8 r5:ca2fa000
[   92.311325]  r4:00000555
[   92.311382] [<bf157000>] (test_bpf_init [test_bpf]) from
[<c0301f7c>] (do_one_initcall+0x4c/0x174)
[   92.311468]  r10:bf154640 r9:c61c2524 r8:39e3db1c r7:00000001
r6:00000000 r5:bf157000
[   92.311529]  r4:ffffe000
[   92.311575] [<c0301f30>] (do_one_initcall) from [<c042a5b0>]
(do_init_module+0x6c/0x1fc)
[   92.311673]  r9:c61c2524 r8:39e3db1c r6:c61c2480 r5:00000001 r4:bf154640
[   92.311744] [<c042a544>] (do_init_module) from [<c03d393c>]
(load_module+0x1f8c/0x2394)
[   92.311815]  r6:c61c2500 r5:00000001 r4:c61b9f34
[   92.311898] [<c03d19b0>] (load_module) from [<c03d3ea0>]
(SyS_init_module+0x15c/0x174)
[   92.311979]  r10:00000051 r9:00000000 r8:00160fda r7:c61b8000
r6:c95a6a18 r5:b6fbca20
[   92.312040]  r4:00006a18
[   92.312087] [<c03d3d44>] (SyS_init_module) from [<c0308260>]
(ret_fast_syscall+0x0/0x3c)
[   92.312196]  r10:00000000 r9:c61b8000 r8:c0308424 r7:00000080
r6:756e694c r5:00156a18
[   92.312277]  r4:00000000
[   93.835343] 1065840 PASS

Does this look like a bug? I will send the separate mail if it does.
Let me know.

Best,
Shubham Bansal


On Tue, May 23, 2017 at 1:35 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, May 22, 2017 at 10:04 AM, Shubham Bansal
> <illusionist.neo@gmail.com> wrote:
>> These all benchmarks are for ARMv7.
>
> Thanks! In the future, try to avoid the white-space damage
> (line-wrapping). And it looks like you've still got debugging turned
> on in your jit code:
>
> [   56.176033] test_bpf: #21 LD_CPU
> [   56.176329] bpf_jit: *** NOT YET: opcode 85 ***
> [   56.176565] jited:0 2639 702 PASS
>
> That breaks the test report line. After I cleaned these up and parsed
> the results, they look great. Most things are half the speed of the
> interpreter, if not better. Only the LD_ABS suffered, and that's
> mainly the const blinding, I assume.
>
> Please post your current patch. Thanks for this!
>
> -Kees
>
> --
> Kees Cook
> Pixel Security

^ permalink raw reply


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