* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Andrew Lunn @ 2017-05-22 21:02 UTC (permalink / raw)
To: Timur Tabi
Cc: Zefir Kurtisi, netdev, f.fainelli, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <66de55da-7a5c-68b8-50d5-cab313ec0d6f@codeaurora.org>
> 2. I'm preparing a patch that adds a command-line parameter to at803x that
> makes this code conditional.
FYI:
A patch with a command line argument, i think you actually mean a
module argument, is very likely to be rejected.
Andrew
^ permalink raw reply
* Re: 4.12-RC2 BUG: scheduling while atomic: irq/47-iwlwifi
From: Arend Van Spriel @ 2017-05-22 21:02 UTC (permalink / raw)
To: Johannes Berg, Sander Eikelenboom
Cc: linux-wireless, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <764a929c-ce8a-c859-a49e-2f20cb05ae44-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
On 22-5-2017 14:09, Arend van Spriel wrote:
> On 5/22/2017 12:57 PM, Johannes Berg wrote:
>> On Mon, 2017-05-22 at 12:36 +0200, Sander Eikelenboom wrote:
>>> Hi,
>>>
>>> I encountered this splat with 4.12-RC2.
>>
>> Ugh, yeah, I should've seen that in the review.
>>
>> Arend, please take a look at this. cfg80211_sched_scan_results() cannot
>> sleep, so you can't rtnl_lock() in there. Looks like you can just rely
>> on RCU though?
>
> I see. I think you are right on RCU. Don't have the code in front of me
> now, but I think the lookup has an ASSERT_RTNL. Will look into it after
> my monday meeting :-p
I realized I have a laptop lying around with intel 3160 wifi chip and
tried to reproduce the issue. Did not run into the splat running
4.12-rc1 from wireless-drivers-next repo. I did not get the email from
Sander so I don't know any details.
Here is what I changed based on the info Johannes provided. Can you
please check if this get rid of the splat and let me know.
Regards,
Arend
---
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 14d5f0c..04833bb 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -322,9 +322,7 @@ static void cfg80211_del_sched_scan_req(struct
cfg80211_regi
{
struct cfg80211_sched_scan_request *pos;
- ASSERT_RTNL();
^ permalink raw reply related
* Re: 4.12-RC2 BUG: scheduling while atomic: irq/47-iwlwifi
From: Johannes Berg @ 2017-05-22 21:04 UTC (permalink / raw)
To: Arend Van Spriel, Sander Eikelenboom; +Cc: linux-wireless, netdev
In-Reply-To: <532c257e-52a0-18c1-1afe-04d37c28e072@broadcom.com>
Hi Arend,
Sorry, I forgot that the original message wasn't Cc'ed to the wireless
list, only netdev.
> +++ b/net/wireless/scan.c
> @@ -322,9 +322,7 @@ static void cfg80211_del_sched_scan_req(struct
> cfg80211_regi
> {
> struct cfg80211_sched_scan_request *pos;
>
> - ASSERT_RTNL();
> -
> - list_for_each_entry(pos, &rdev->sched_scan_req_list, list) {
> + list_for_each_entry_rcu(pos, &rdev->sched_scan_req_list,
> list) {
[snip]
This looks fine, but perhaps in the above we should have some kind of
locking assertion, e.g.
WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_rtnl_is_held());
johannes
^ permalink raw reply
* Re: [patch net-next 2/2] net/sched: fix filter flushing
From: Cong Wang @ 2017-05-22 21:04 UTC (permalink / raw)
To: Jiri Pirko
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Eric Dumazet, Daniel Borkmann, Simon Horman, mlxsw, Colin King
In-Reply-To: <CAM_iQpUj8C+Rt_Jm8J_DHXedHAg-_GaKUYgQwQ28hN-=zcB-5Q@mail.gmail.com>
On Mon, May 22, 2017 at 1:54 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, May 21, 2017 at 12:19 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>You can't claim you really delete it as long as actions can still
>>>see it and dump it.
>>
>> No, user just wants to delete all the filters. That is done. User does
>> not care if the actual chain structure is there or not.
>>
>
> Hmm, so users see a chain with no filters... Fair enough.
But since you remove the chain from the chain_list, it means
users could not add new filters to this chain after flushing? And
users could create a new chain with the same index??
If so, you should instead keep it in the chain_list, although empty.
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Andrew Lunn @ 2017-05-22 21:09 UTC (permalink / raw)
To: Timur Tabi
Cc: Zefir Kurtisi, netdev, f.fainelli, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <66de55da-7a5c-68b8-50d5-cab313ec0d6f@codeaurora.org>
On Mon, May 22, 2017 at 03:12:03PM -0500, Timur Tabi wrote:
> On 10/24/2016 05:40 AM, Zefir Kurtisi wrote:
> > This commit adds a wrapper function for at8031
> > that in case of operating in SGMII mode double
> > checks SGMII link state when generic aneg_done()
> > succeeds. It prints a warning on failure but
> > intentionally does not try to recover from this
> > state. As a result, if you ever see a warning
> > '803x_aneg_done: SGMII link is not ok' you will
> > end up having an Ethernet link up but won't get
> > any data through. This should not happen, if it
> > does, please contact the module maintainer.
>
> I'm getting bitten by this one again. We're now have several systems that
> are reporting the link failure ("803x_aneg_done: SGMII link is not ok"), and
> the interface comes up but is not functional. I believe this is expected.
>
> The problem, however, is not because of the link failure. Instead, the
> problem is this:
>
> > + /* check if the SGMII link is OK. */
> > + if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) {
> > + pr_warn("803x_aneg_done: SGMII link is not ok\n");
> > + aneg_done = 0;
>
> Returning zero is what breaks the interface. If I comment-out this last
> line, so that at803x_aneg_done() returns BMSR_ANEGCOMPLETE instead, then
> everything works.
Are you using interrupts? Or polling?
If polling, it should come back again 1 second later and see if
auto-neg has completed. Hopefully the SGMII side comes up eventually.
If you are using interrupts, you need another interrupt when the SGMII
side comes up, otherwise i think the state machine is stuck waiting.
Andrew
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Florian Fainelli @ 2017-05-22 21:10 UTC (permalink / raw)
To: Andrew Lunn, Timur Tabi
Cc: Zefir Kurtisi, netdev, David Miller, Manoj Iyer, jhugo
In-Reply-To: <20170522210215.GQ29447@lunn.ch>
On 05/22/2017 02:02 PM, Andrew Lunn wrote:
>> 2. I'm preparing a patch that adds a command-line parameter to at803x that
>> makes this code conditional.
>
> FYI:
>
> A patch with a command line argument, i think you actually mean a
> module argument, is very likely to be rejected.
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
- 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)
Preference goes for the first solution, but phy_flags is just a 32-bits
integer, so you could run out of bits.
--
Florian
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-05-22 21:19 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: Zefir Kurtisi, netdev, David Miller, Manoj Iyer, jhugo
In-Reply-To: <0528e1fa-2924-1286-578d-060419bfee49@gmail.com>
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? I call phy_connect_direct() instead
of of_phy_connect(). I see some drivers set phydev->dev_flags before
calling phy_connect_direct().
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?
> - 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?
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH net-next] net: make struct inet_frags::qsize unsigned
From: Alexey Dobriyan @ 2017-05-22 21:20 UTC (permalink / raw)
To: davem; +Cc: netdev
This field is sizeof of corresponding kmem_cache so it can't be negative.
Prepare for 32-bit kmem_cache_create().
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
include/net/inet_frag.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -92,7 +92,7 @@ struct inet_frags {
*/
u32 rnd;
seqlock_t rnd_seqlock;
- int qsize;
+ unsigned int qsize;
unsigned int (*hashfn)(const struct inet_frag_queue *);
bool (*match)(const struct inet_frag_queue *q,
^ permalink raw reply
* RE: [PATCH v3 net-next 3/5] dsa: add DSA switch driver for Microchip KSZ9477
From: Woojung.Huh @ 2017-05-22 21:21 UTC (permalink / raw)
To: andrew
Cc: f.fainelli, vivien.didelot, sergei.shtylyov, netdev, davem,
UNGLinuxDriver
In-Reply-To: <20170522143134.GC29447@lunn.ch>
Hi Andres,
> > +static struct {
> > + int index;
> > + char string[ETH_GSTRING_LEN];
>
> Hi Woojung
>
> Since you need to respin for the skb_put_padto(), please make this
> const.
OK.
> > +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?
- Woojung
^ permalink raw reply
* [PATCH] net: make struct request_sock_ops::obj_size unsigned
From: Alexey Dobriyan @ 2017-05-22 21:21 UTC (permalink / raw)
To: davem; +Cc: netdev
This field is sizeof of corresponding kmem_cache so it can't be negative.
Space will be saved after 32-bit kmem_cache_create() patch.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
include/net/request_sock.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -29,7 +29,7 @@ struct proto;
struct request_sock_ops {
int family;
- int obj_size;
+ unsigned int obj_size;
struct kmem_cache *slab;
char *slab_name;
int (*rtx_syn_ack)(const struct sock *sk,
^ permalink raw reply
* Re: [PATCH] net/core: remove explicit do_softirq() from busy_poll_stop()
From: Eric Dumazet @ 2017-05-22 21:26 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: netdev, David S. Miller, Thomas Gleixner
In-Reply-To: <20170522192641.hylqmhb7t2fykk5e@linutronix.de>
On Mon, May 22, 2017 at 12:26 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Since commit 217f69743681 ("net: busy-poll: allow preemption in
> sk_busy_loop()") there is an explicit do_softirq() invocation after
> local_bh_enable() has been invoked.
> I don't understand why we need this because local_bh_enable() will
> invoke do_softirq() once the softirq counter reached zero and we have
> softirq-related work pending.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> net/core/dev.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fca407b4a6ea..e84eb0ec5529 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5199,8 +5199,6 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock)
> if (rc == BUSY_POLL_BUDGET)
> __napi_schedule(napi);
> local_bh_enable();
> - if (local_softirq_pending())
> - do_softirq();
> }
preemption is disabled.
Look at netif_rx_ni() for a similar construct.
What exact problem do you have with existing code, that is worth
adding this change ?
Thanks.
^ permalink raw reply
* Re: [PATCH v3 net-next 3/5] dsa: add DSA switch driver for Microchip KSZ9477
From: Andrew Lunn @ 2017-05-22 21:27 UTC (permalink / raw)
To: Woojung.Huh
Cc: f.fainelli, vivien.didelot, sergei.shtylyov, netdev, davem,
UNGLinuxDriver
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A81210@CHN-SV-EXMX02.mchp-main.com>
On Mon, May 22, 2017 at 09:21:28PM +0000, Woojung.Huh@microchip.com wrote:
> Hi Andres,
>
> > > +static struct {
> > > + int index;
> > > + char string[ETH_GSTRING_LEN];
> >
> > Hi Woojung
> >
> > Since you need to respin for the skb_put_padto(), please make this
> > const.
> OK.
>
> > > +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
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-05-22 21:29 UTC (permalink / raw)
To: Andrew Lunn
Cc: Zefir Kurtisi, netdev, f.fainelli, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <20170522210927.GR29447@lunn.ch>
On 05/22/2017 04:09 PM, Andrew Lunn wrote:
> Are you using interrupts? Or polling?
adpt->phydev->irq = PHY_IGNORE_INTERRUPT;
ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
PHY_INTERFACE_MODE_SGMII);
Technically it's polling, except that it's my NIC's hardware that is polling
the MDIO bus, and then generating an interrupt when there's a link state change.
When the link state changes, I call phy_mac_interrupt()
if (status & ISR_GPHY_LINK)
phy_mac_interrupt(adpt->phydev, !!(status & GPHY_LINK_UP_INT));
> If polling, it should come back again 1 second later and see if
> auto-neg has completed. Hopefully the SGMII side comes up eventually.
>
> If you are using interrupts, you need another interrupt when the SGMII
> side comes up, otherwise i think the state machine is stuck waiting.
I'll have to test this, but what do I do if I don't get another interrupt?
I have a suspicion that the link is actually okay, and that the error is bogus.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [RFC net-next PATCH 4/5] net: new XDP feature for reading HW rxhash from drivers
From: Tom Herbert @ 2017-05-22 21:32 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov,
Linux Kernel Network Developers
In-Reply-To: <20170522224229.540ce059@redhat.com>
On Mon, May 22, 2017 at 1:42 PM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Mon, 22 May 2017 08:39:35 +0200
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
>> On Sun, 21 May 2017 15:10:29 -0700
>> Tom Herbert <tom@herbertland.com> wrote:
>>
>> > On Sun, May 21, 2017 at 9:04 AM, Jesper Dangaard Brouer
>> > <brouer@redhat.com> wrote:
>> > > On Sat, 20 May 2017 09:16:09 -0700
>> > > Tom Herbert <tom@herbertland.com> wrote:
>> > >
>> > >> > +/* XDP rxhash have an associated type, which is related to the RSS
>> > >> > + * (Receive Side Scaling) standard, but NIC HW have different mapping
>> > >> > + * and support. Thus, create mapping that is interesting for XDP. XDP
>> > >> > + * would primarly want insight into L3 and L4 protocol info.
>> > >> > + *
>> > >> > + * TODO: Likely need to get extended with "L3_IPV6_EX" due RSS standard
>> > >> > + *
>> > >> > + * The HASH_TYPE will be returned from bpf helper as the top 32-bit of
>> > >> > + * the 64-bit rxhash (internally type stored in xdp_buff->flags).
>> > >> > + */
>> > >> > +#define XDP_HASH(x) ((x) & ((1ULL << 32)-1))
>> > >> > +#define XDP_HASH_TYPE(x) ((x) >> 32)
>> > >> > +
>> > >> > +#define XDP_HASH_TYPE_L3_SHIFT 0
>> > >> > +#define XDP_HASH_TYPE_L3_BITS 3
>> > >> > +#define XDP_HASH_TYPE_L3_MASK ((1ULL << XDP_HASH_TYPE_L3_BITS)-1)
>> > >> > +#define XDP_HASH_TYPE_L3(x) ((x) & XDP_HASH_TYPE_L3_MASK)
>> > >> > +enum {
>> > >> > + XDP_HASH_TYPE_L3_IPV4 = 1,
>> > >> > + XDP_HASH_TYPE_L3_IPV6,
>> > >> > +};
>> > >> > +
>> > >> > +#define XDP_HASH_TYPE_L4_SHIFT XDP_HASH_TYPE_L3_BITS
>> > >> > +#define XDP_HASH_TYPE_L4_BITS 5
>> > >> > +#define XDP_HASH_TYPE_L4_MASK \
>> > >> > + (((1ULL << XDP_HASH_TYPE_L4_BITS)-1) << XDP_HASH_TYPE_L4_SHIFT)
>> > >> > +#define XDP_HASH_TYPE_L4(x) ((x) & XDP_HASH_TYPE_L4_MASK)
>> > >> > +enum {
>> > >> > + _XDP_HASH_TYPE_L4_TCP = 1,
>> > >> > + _XDP_HASH_TYPE_L4_UDP,
>> > >> > +};
>> > >> > +#define XDP_HASH_TYPE_L4_TCP (_XDP_HASH_TYPE_L4_TCP << XDP_HASH_TYPE_L4_SHIFT)
>> > >> > +#define XDP_HASH_TYPE_L4_UDP (_XDP_HASH_TYPE_L4_UDP << XDP_HASH_TYPE_L4_SHIFT)
>> > >> > +
>> > >> Hi Jesper,
>> > >>
>> > >> Why do we need these indicators for protocol specific hash? It seems
>> > >> like L4 and L3 is useful differentiation and protocol agnostic (I'm
>> > >> still holding out hope that SCTP will be deployed some day ;-) )
>> > >
>> > > I'm not sure I understood the question fully, but let me try to answer
>> > > anyway. To me it seems obvious that you would want to know the
>> > > protocol/L4 type, as this helps avoid hash collisions between UDP and
>> > > TCP flows. I can easily imagine someone constructing an UDP packet
>> > > that could hash collide with a given TCP flow.
>> > >
>> > > And yes, i40 support matching SCTP, and we will create a
>> > > XDP_HASH_TYPE_L4_SCTP when adding XDP rxhash support for that driver.
>> > >
>> > But where would this information be used? We don't save it in skbuff,
>> > don't use it in RPS, RFS. RSS doesn't use it for packet steering so
>> > the hash collision problem already exists at the device level. If
>> > there is a collision problem between two protocols then maybe hash
>> > should be over 5-tuple instead...
>>
>> One use-case (I heard at a customer) was that they had (web-)servers
>> that didn't serve any UDP traffic, thus they simply block/drop all
>> incoming UDP on the service NIC (as an ACL in the switch). (The servers
>> own DNS lookups and NTP goes through the management NIC to internal
>> DNS/NTP servers).
>>
>> Another use-case: Inside an XDP/bpf program is can be used for
>> splitting protocol processing, into different tail calls, before even
>> touching packet-data. I can imagine the bpf TCP handling code is
>> larger, thus an optimization is to have a separate tail call for the
>> UDP protocol handling. One could also transfer/queue all TCP traffic
>> to other CPU(s) like RPS, just without touching packet memory.
>>
>>
>> This info is saved in the skb, but due to space constrains, it is
>> reduced to a single bit, namely skb->l4_hash, iif some
>> RSS-proto/XDP_HASH_TYPE_L4_* bit was set. And the network stack do use
>> and react on this.
>
> I also want to mention another real-customer use-case. Some
> deployments have a VXLAN based networks, but some NICs cannot
> understand VXLAN do cannot do proper RSS rx-hashing, which resulted in
> bad CPU scaling as all VXLAN packets gets delivered to the same CPU.
>
They need to turn on RSS for UDP I think.
> Thus, I would like to implement recalculation of the RXHASH in XDP, as
> that would save me implementing yet another extension to the flow
> dissector, that the kernel would have to carry forever, while this is
> just a matter of NIC hashing getting improved.
>
> With the extra L3 and L4 info, I'm assuming that XDP_HASH_TYPE_L3(x)
> and XDP_HASH_TYPE_L4(x) will be zero for VXLAN as the NIC cannot
> identify this. Thus, I can at an early stage know which packets needs
> to get a new rxhash.
>
> I've seen a similar problem with Q-in-Q double tagged VLANs, failing
> the RSS-hash distribution the same way...
>
That's fine, but I'm still not seeing that differentiating the hash
created from different protocols is really helps. The most correct way
moving forward to generate a flow hash is use IPv6 flow label. This
eliminates either the device or software from needing to do DPI into
protocols, over extension headers, etc. In this case though we declare
a hash based on flow label as L4, but have no idea and really don't
care what protocol it's for (and in fact for something like IPsec we
can't even know).
The idea that the world is composed of just TCP and UDP is a delusion
espoused by NIC implementations, in SW we can and should do better
than that with protocol agnostic mechanisms.
Tom
> I hope that explains what this can be use for(?)
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Andrew Lunn @ 2017-05-22 21:32 UTC (permalink / raw)
To: Timur Tabi
Cc: Zefir Kurtisi, netdev, f.fainelli, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <0d9b30e5-40f3-82ba-a44a-2eb25b8d3094@codeaurora.org>
> I'll have to test this, but what do I do if I don't get another interrupt?
It probably means interrupts cannot be used. Poll it.
Andrew
^ permalink raw reply
* 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
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