* [PATCH 2/2] staging: rtl8712u: Fix compiler warning about strncpy
From: Larry Finger @ 2018-08-20 17:51 UTC (permalink / raw)
To: gregkh; +Cc: netdev, devel, Larry Finger
In-Reply-To: <20180820175124.23863-1-Larry.Finger@lwfinger.net>
When strncpy() is called with source and destination strings the same
length, gcc 8 warns that there may be an unterminated string. Using
strlcpy() rather than strncpy() forces a null at the end and quiets the
warning.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index c3ff7c3e6681..08e1c0957a07 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -1789,7 +1789,7 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
return -ENOMEM;
param->cmd = IEEE_CMD_SET_ENCRYPTION;
eth_broadcast_addr(param->sta_addr);
- strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
+ strlcpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
if (pext->ext_flags & IW_ENCODE_EXT_GROUP_KEY)
param->u.crypt.set_tx = 0;
if (pext->ext_flags & IW_ENCODE_EXT_SET_TX_KEY)
--
2.18.0
^ permalink raw reply related
* [PATCH 1/2] staging: rtl8192e: Fix compiler warning about strncpy
From: Larry Finger @ 2018-08-20 17:51 UTC (permalink / raw)
To: gregkh; +Cc: netdev, devel, Larry Finger
In-Reply-To: <20180820175124.23863-1-Larry.Finger@lwfinger.net>
When strncpy() is called with source and destination strings the same
length, gcc 8 warns that there may be an unterminated string. Using
strlcpy() rather than strncpy() forces a null at the end and quiets the
warning.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
drivers/staging/rtl8192e/rtllib_softmac.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 919231fec09c..95a8390cb7ac 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -1684,14 +1684,14 @@ inline void rtllib_softmac_new_net(struct rtllib_device *ieee,
* essid provided by the user.
*/
if (!ssidbroad) {
- strncpy(tmp_ssid, ieee->current_network.ssid,
+ strlcpy(tmp_ssid, ieee->current_network.ssid,
IW_ESSID_MAX_SIZE);
tmp_ssid_len = ieee->current_network.ssid_len;
}
memcpy(&ieee->current_network, net,
sizeof(struct rtllib_network));
if (!ssidbroad) {
- strncpy(ieee->current_network.ssid, tmp_ssid,
+ strlcpy(ieee->current_network.ssid, tmp_ssid,
IW_ESSID_MAX_SIZE);
ieee->current_network.ssid_len = tmp_ssid_len;
}
--
2.18.0
^ permalink raw reply related
* [PATCH 0/2] staging: Fix some warnings from strncpy()
From: Larry Finger @ 2018-08-20 17:51 UTC (permalink / raw)
To: gregkh; +Cc: netdev, devel, Larry Finger
When the size argument in a call to strncpy() is the size of the
destimation, gcc8 issues a warning. These patches fix the potential
problem by replacing the strncpy() with strlcpy().
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Larry Finger (2):
staging: rtl8192e: Fix compiler warning about strncpy
staging: rtl8712u: Fix compiler warning about strncpy
drivers/staging/rtl8192e/rtllib_softmac.c | 4 ++--
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
--
2.18.0
^ permalink raw reply
* Re: [PATCH] r8169: don't use MSI-X on RTL8106e
From: Heiner Kallweit @ 2018-08-20 20:56 UTC (permalink / raw)
To: Florian Fainelli, David Miller
Cc: jian-hong, nic_swsd, netdev, linux-kernel, linux
In-Reply-To: <4bf216ab-69ea-c09e-dcd8-f033a13627c0@gmail.com>
On 20.08.2018 22:40, Florian Fainelli wrote:
> On 08/16/2018 12:50 PM, Heiner Kallweit wrote:
>> On 16.08.2018 21:39, David Miller wrote:
>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>> Date: Thu, 16 Aug 2018 21:37:31 +0200
>>>
>>>> On 16.08.2018 21:21, David Miller wrote:
>>>>> From: <jian-hong@endlessm.com>
>>>>> Date: Wed, 15 Aug 2018 14:21:10 +0800
>>>>>
>>>>>> Found the ethernet network on ASUS X441UAR doesn't come back on resume
>>>>>> from suspend when using MSI-X. The chip is RTL8106e - version 39.
>>>>>
>>>>> Heiner, please take a look at this.
>>>>>
>>>>> You recently disabled MSI-X on RTL8168g for similar reasons.
>>>>>
>>>>> Now that we've seen two chips like this, maybe there is some other
>>>>> problem afoot.
>>>>>
>>>> Thanks for the hint. I saw it already and just contacted Realtek
>>>> whether they are aware of any MSI-X issues with particular chip
>>>> versions. With the chip versions I have access to MSI-X works fine.
>>>>
>>>> There's also the theoretical option that the issues are caused by
>>>> broken BIOS's. But so far only chip versions have been reported
>>>> which are very similar, at least with regard to version number
>>>> (2x VER_40, 1x VER_39). So they may share some buggy component.
>>>>
>>>> Let's see whether Realtek can provide some hint.
>>>> If more chip versions are reported having problems with MSI-X,
>>>> then we could switch to a whitelist or disable MSI-X in general.
>>>
>>> It could be that we need to reprogram some register(s) on resume,
>>> which normally might not be needed, and that is what is causing the
>>> problem with some chips.
>>>
>> Indeed. That's what I'm checking with Realtek.
>> In the register list in the r8169 driver there's one entry which
>> seems to indicate that there are MSI-X specific settings.
>> However this register isn't used, and the r8168 vendor driver
>> uses only MSI. And there are no public datasheets.
>
> Stupid question, but should not we be asking the reporter to try again with:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdd19ad80f203f42f05fd32a31c678c9c524ef9
>
> applied? The original report shows the Generic PHY being used, not the
> Realtek PHY driver being used, is this possibly contributing to the problem?
>
I don't think it's related, because falling back to MSI fixes the issue for
the reporter. And some chip versions report a generic Realtek PHY ID which
isn't covered by any Realtek PHY driver. These chip versions seem to work
fine with the generic PHY driver. So he may have Realtek PHY drivers enabled
or not. But indeed, would be good to have this info to get the full picture.
See also the mail I wrote few minutes ago, there it's described what we know
about the reason of the MSI-X issue so far.
^ permalink raw reply
* Re: [PATCH] r8169: don't use MSI-X on RTL8106e
From: Heiner Kallweit @ 2018-08-20 20:46 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: David Miller, jian-hong, nic_swsd, netdev, linux-kernel, linux,
linux-pci, Marc Zyngier, Thomas Gleixner, Christoph Hellwig
In-Reply-To: <20180820184438.GA154536@bhelgaas-glaptop.roam.corp.google.com>
On 20.08.2018 20:44, Bjorn Helgaas wrote:
> [+cc Marc, Thomas, Christoph, linux-pci)
> (beginning of thread at [1])
>
> On Thu, Aug 16, 2018 at 09:50:48PM +0200, Heiner Kallweit wrote:
>> On 16.08.2018 21:39, David Miller wrote:
>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>> Date: Thu, 16 Aug 2018 21:37:31 +0200
>>>
>>>> On 16.08.2018 21:21, David Miller wrote:
>>>>> From: <jian-hong@endlessm.com>
>>>>> Date: Wed, 15 Aug 2018 14:21:10 +0800
>>>>>
>>>>>> Found the ethernet network on ASUS X441UAR doesn't come back on resume
>>>>>> from suspend when using MSI-X. The chip is RTL8106e - version 39.
>>>>>
>>>>> Heiner, please take a look at this.
>>>>>
>>>>> You recently disabled MSI-X on RTL8168g for similar reasons.
>>>>>
>>>>> Now that we've seen two chips like this, maybe there is some other
>>>>> problem afoot.
>>>>>
>>>> Thanks for the hint. I saw it already and just contacted Realtek
>>>> whether they are aware of any MSI-X issues with particular chip
>>>> versions. With the chip versions I have access to MSI-X works fine.
>>>>
>>>> There's also the theoretical option that the issues are caused by
>>>> broken BIOS's. But so far only chip versions have been reported
>>>> which are very similar, at least with regard to version number
>>>> (2x VER_40, 1x VER_39). So they may share some buggy component.
>>>>
>>>> Let's see whether Realtek can provide some hint.
>>>> If more chip versions are reported having problems with MSI-X,
>>>> then we could switch to a whitelist or disable MSI-X in general.
>>>
>>> It could be that we need to reprogram some register(s) on resume,
>>> which normally might not be needed, and that is what is causing the
>>> problem with some chips.
>>>
>> Indeed. That's what I'm checking with Realtek.
>> In the register list in the r8169 driver there's one entry which
>> seems to indicate that there are MSI-X specific settings.
>> However this register isn't used, and the r8168 vendor driver
>> uses only MSI. And there are no public datasheets.
>
> Do we have any information about these chip versions in other systems?
> Or other devices using MSI-X in the same ASUS system? It seems
> possible that there's some PCI core or suspend/resume issue with MSI-X
> and this patch just avoids it without fixing the root cause.
>
I'm in contact with Realtek and according to them few chip versions
seem to clear MSI-X table entries on resume from suspend. Checking
with them how this could be fixed / worked around.
Worst case we may have to disable MSI-X in general.
> It might be useful to have a kernel.org bugzilla with the complete
> dmesg, "sudo lspci -vv" output, and /proc/interrupts contents archived
> for future reference.
>
> [1] https://lkml.kernel.org/r/20180815062110.16155-1-jian-hong@endlessm.com
>
^ permalink raw reply
* Re: [PATCH] r8169: don't use MSI-X on RTL8106e
From: Florian Fainelli @ 2018-08-20 20:40 UTC (permalink / raw)
To: Heiner Kallweit, David Miller
Cc: jian-hong, nic_swsd, netdev, linux-kernel, linux
In-Reply-To: <ac30d4e6-6411-09e6-1205-96e3e8e29f6f@gmail.com>
On 08/16/2018 12:50 PM, Heiner Kallweit wrote:
> On 16.08.2018 21:39, David Miller wrote:
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Date: Thu, 16 Aug 2018 21:37:31 +0200
>>
>>> On 16.08.2018 21:21, David Miller wrote:
>>>> From: <jian-hong@endlessm.com>
>>>> Date: Wed, 15 Aug 2018 14:21:10 +0800
>>>>
>>>>> Found the ethernet network on ASUS X441UAR doesn't come back on resume
>>>>> from suspend when using MSI-X. The chip is RTL8106e - version 39.
>>>>
>>>> Heiner, please take a look at this.
>>>>
>>>> You recently disabled MSI-X on RTL8168g for similar reasons.
>>>>
>>>> Now that we've seen two chips like this, maybe there is some other
>>>> problem afoot.
>>>>
>>> Thanks for the hint. I saw it already and just contacted Realtek
>>> whether they are aware of any MSI-X issues with particular chip
>>> versions. With the chip versions I have access to MSI-X works fine.
>>>
>>> There's also the theoretical option that the issues are caused by
>>> broken BIOS's. But so far only chip versions have been reported
>>> which are very similar, at least with regard to version number
>>> (2x VER_40, 1x VER_39). So they may share some buggy component.
>>>
>>> Let's see whether Realtek can provide some hint.
>>> If more chip versions are reported having problems with MSI-X,
>>> then we could switch to a whitelist or disable MSI-X in general.
>>
>> It could be that we need to reprogram some register(s) on resume,
>> which normally might not be needed, and that is what is causing the
>> problem with some chips.
>>
> Indeed. That's what I'm checking with Realtek.
> In the register list in the r8169 driver there's one entry which
> seems to indicate that there are MSI-X specific settings.
> However this register isn't used, and the r8168 vendor driver
> uses only MSI. And there are no public datasheets.
Stupid question, but should not we be asking the reporter to try again with:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdd19ad80f203f42f05fd32a31c678c9c524ef9
applied? The original report shows the Generic PHY being used, not the
Realtek PHY driver being used, is this possibly contributing to the problem?
--
Florian
^ permalink raw reply
* Re: [RFC v3 net-next 4/5] rds: invoke socket sg filter attached to rds socket
From: Santosh Shilimkar @ 2018-08-20 16:58 UTC (permalink / raw)
To: Tushar Dave, daniel, davem, sowmini.varadhan, jakub.kicinski,
quentin.monnet, jiong.wang, sandipan, kafai, rdna
Cc: john.fastabend, ast, yhs, netdev
In-Reply-To: <1534547305-25140-5-git-send-email-tushar.n.dave@oracle.com>
On 8/17/2018 4:08 PM, Tushar Dave wrote:
> RDS module sits on top of TCP (rds_tcp) and IB (rds_rdma), so messages
> arrive in form of skb (over TCP) and scatterlist (over IB/RDMA).
> However, because socket filter only deal with skb (e.g. struct skb as
> bpf context) we can only use socket filter for rds_tcp and not for
> rds_rdma.
>
> Considering one filtering solution for RDS, it seems that the common
> denominator between sk_buff and scatterlist is scatterlist. Therefore,
> this patch converts skb to sgvec and invoke sg_filter_run for
> rds_tcp and simply invoke sg_filter_run for IB/rds_rdma.
>
> Signed-off-by: Tushar Dave <tushar.n.dave@oracle.com>
> Reviewed-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
Looks good Tushar !!
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
* Re: Allocation failure with subsequent kernel crash
From: Daniel Borkmann @ 2018-08-20 19:29 UTC (permalink / raw)
To: whiteheadm, Alexei Starovoitov
Cc: LKML, Network Development, Ingo Molnar, Thomas Gleixner,
Alexei Starovoitov
In-Reply-To: <CAP8WD_ba1k6uxSFQ00QtW0L23jzjSFw93YpEq+010zKE5sT27g@mail.gmail.com>
Hi Matthew,
On 08/20/2018 08:03 AM, tedheadster wrote:
> On Mon, Aug 20, 2018 at 1:22 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> I don't remember vzalloc issues that was fixed in this area.
>> 4.14 kernel is quite old. Since then syzbot found few mem
>> related bugs that were fixed.
>> please try to reproduce on the latest tree.
>
> Alexei,
> I get this panic with two very recent kernels: 4.18.0 and 4.17.14. I
> do not think this has been fixed. I am still trying to bisect it, but
> sometimes it takes 5 hours for the panic to occur.
I've been looking into it a bit today and still am. Given you've seen
this on x86_32 and also on older kernels, I presume JIT was not involved
(/proc/sys/net/core/bpf_jit_enable is 0). Do you run any specific workload
until you trigger this (e.g. fuzzer on BPF), or any specific event that
triggers at that time after ~5hrs? Or only systemd on idle machine? Have
you managed to reproduce this also elsewhere? Bisect seems indeed painful
but would help tremendously; perhaps also dumping the BPF insns that are
loaded at that point in time.
Thanks a lot,
Daniel
> - Matthew Whitehead
>
^ permalink raw reply
* Re: [bpf-next RFC 1/3] flow_dissector: implements flow dissector BPF hook
From: Song Liu @ 2018-08-20 16:04 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Petar Penkov, Petar Penkov, Network Development, David Miller,
Alexei Starovoitov, Daniel Borkmann, simon.horman,
Willem de Bruijn
In-Reply-To: <CAF=yD-KciJUH6Mi_oE2rqfOPWTLvEAdinos64fL=0+dEA=_gFQ@mail.gmail.com>
On Mon, Aug 20, 2018 at 7:13 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Mon, Aug 20, 2018 at 1:47 AM Song Liu <liu.song.a23@gmail.com> wrote:
>>
>> On Thu, Aug 16, 2018 at 4:14 PM, Petar Penkov <ppenkov@google.com> wrote:
>> > On Thu, Aug 16, 2018 at 3:40 PM, Song Liu <liu.song.a23@gmail.com> wrote:
>> >>
>> >> On Thu, Aug 16, 2018 at 9:44 AM, Petar Penkov <peterpenkov96@gmail.com> wrote:
>> >> > From: Petar Penkov <ppenkov@google.com>
>> >> >
>> >> > Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
>> >> > attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
>> >> > path. The BPF program is kept as a global variable so it is
>> >> > accessible to all flow dissectors.
>> >> >
>> >> > Signed-off-by: Petar Penkov <ppenkov@google.com>
>> >> > Signed-off-by: Willem de Bruijn <willemb@google.com>
>
>> >> > @@ -658,6 +698,42 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> >> > FLOW_DISSECTOR_KEY_BASIC,
>> >> > target_container);
>> >> >
>> >> > + rcu_read_lock();
>> >> > + attached = rcu_dereference(flow_dissector_prog);
>> >> > + if (attached) {
>> >> > + /* Note that even though the const qualifier is discarded
>> >> > + * throughout the execution of the BPF program, all changes(the
>> >> > + * control block) are reverted after the BPF program returns.
>> >> > + * Therefore, __skb_flow_dissect does not alter the skb.
>> >> > + */
>> >> > + struct bpf_flow_dissect_cb *cb;
>> >> > + u8 cb_saved[BPF_SKB_CB_LEN];
>> >> > + u32 result;
>> >> > +
>> >> > + cb = (struct bpf_flow_dissect_cb *)(bpf_skb_cb((struct sk_buff *)skb));
>> >> > +
>> >> > + /* Save Control Block */
>> >> > + memcpy(cb_saved, cb, sizeof(cb_saved));
>> >> > + memset(cb, 0, sizeof(cb_saved));
>> >> > +
>> >> > + /* Pass parameters to the BPF program */
>> >> > + cb->nhoff = nhoff;
>> >> > + cb->target_container = target_container;
>> >> > + cb->flow_dissector = flow_dissector;
>> >> > +
>> >> > + bpf_compute_data_pointers((struct sk_buff *)skb);
>> >> > + result = BPF_PROG_RUN(attached, skb);
>> >> > +
>> >> > + /* Restore state */
>> >> > + memcpy(cb, cb_saved, sizeof(cb_saved));
>> >> > +
>> >> > + key_control->thoff = min_t(u16, key_control->thoff,
>> >> > + skb ? skb->len : hlen);
>> >> > + rcu_read_unlock();
>> >> > + return result == BPF_OK;
>> >> > + }
>> >>
>> >> If the BPF program cannot handle certain protocol, shall we fall back
>> >> to the built-in logic? Otherwise, all BPF programs need to have some
>> >> code for all protocols.
>> >>
>> >> Song
>> >
>> > I believe that if we fall back to the built-in logic we lose all security
>> > guarantees from BPF and this is why the code does not support
>> > fall back.
>> >
>> > Petar
>>
>> I am not really sure we are on the same page. I am proposing 3
>> different return values from BPF_PROG_RUN(), and they should be
>> handled as
>>
>> 1. result == BPF_OK => return true;
>> 2. result == BPF_DROP => return false;
>> 3. result == something else => fall back.
>>
>> Does this proposal make any sense?
>>
>> Thanks,
>> Song
>
> It certainly makes sense. We debated it initially, as well.
>
> In the short term, it allows for simpler BPF programs, as they can
> off-load some protocols to the C implementation.
>
> But the RFC patchset already implements most protocols in BPF.
> I had not expected that when we started out.
>
> Eventually, I think it is preferable to just deprecate the C
> implementation. Which is not possible if we make this opt-out
> a part of the BPF flow dissector interface.
>
> There is also the lesser issue that a buggy BPF program might
> accidentally pass the third value and unknowing open itself up
> to the large attack surface. Without this option, the security
> audit is much simpler.
Thanks for the explanation. I didn't realize that the end goal is to
deprecate the C implementation.
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply
* Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs
From: Andrew Lunn @ 2018-08-20 19:06 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: David S. Miller, Nicolas Ferre, kernel, netdev, mdf, Brad Mouring,
Florian Fainelli, stable
In-Reply-To: <b5445ad9-70bd-12fe-a8e7-1dc3ec73585e@pengutronix.de>
On Mon, Aug 20, 2018 at 05:56:53PM +0200, Ahmad Fatoum wrote:
> On 08/20/2018 03:55 PM, Andrew Lunn wrote:
> > Why is of_phy_register_fixed_link(np) failing?
>
> Apparently, the fixed-link's gpio's FLAG_REQUESTED bit remained set
> causing gpiod_request_commit to return -EBUSY in (v4.18.0):
>
> [<c0667750>] (gpiod_request_commit) from [<c066890c>] (gpiod_request+0x64/0x88)
> [<c066890c>] (gpiod_request) from [<c066a338>] (gpiod_get_from_of_node+0x48/0x13c)
> [<c066a338>] (gpiod_get_from_of_node) from [<c094b92c>] (mdiobus_register_device+0x70/0x124)
> [<c094b92c>] (mdiobus_register_device) from [<c0949d2c>] (phy_device_register+0xc/0xa0)
> [<c0949d2c>] (phy_device_register) from [<c094e824>] (fixed_phy_register+0xe8/0x1f8)
> [<c094e824>] (fixed_phy_register) from [<c0b84260>] (of_phy_register_fixed_link+0x150/0x1e4)
> [<c0b84260>] (of_phy_register_fixed_link) from [<c0964908>] (macb_probe+0x548/0xa7c)
> [<c0964908>] (macb_probe) from [<c086ebec>] (platform_drv_probe+0x48/0x98)
>
> But that's a separate issue, I'll remove this line from the commit message...
I would actually say, this is your real issue here. The warnings are
annoying, but i don't think they are fatal. This -EBUSY is what is
stopping the driver from loading, causing the real regression.
I'm guessing, but i think you will find the driver is loading once,
but hits a EPROBE_DEFFER condition, after getting the gpio. It does
not release the gpio correctly. Sometime later, it gets loaded again,
but the gpio is now in use, so you get the -EBUSY.
So check the error paths, and make sure cleanup is being done correct.
It could also be a phylib core bug...
Andrew
^ permalink raw reply
* Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API
From: Boris Brezillon @ 2018-08-20 19:06 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Srinivas Kandagatla, Alban, Jonathan Corbet, Sekhar Nori,
Kevin Hilman, Russell King, Arnd Bergmann, Greg Kroah-Hartman,
David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
Grygorii Strashko, David S . Miller, Naren, Mauro Carvalho Chehab,
Andrew Morton, Lukas Wunner, Dan Carpenter, Florian
In-Reply-To: <CAMRc=McwEjofZKP93DxPa8vJLE6NceLLGfcbcK9-Pfk-dav_Zg@mail.gmail.com>
On Mon, 20 Aug 2018 20:50:55 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2018-08-20 20:20 GMT+02:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > On Mon, 20 Aug 2018 11:43:34 +0100
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> >
> >>
> >> Overall am still not able to clear visualize on how MTD bindings with
> >> nvmem cells would look in both partition and un-partition usecases?
> >> An example DT would be nice here!!
> >
> > Something along those lines:
> >
> > mtdnode {
> > nvmem-cells {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x14>;
> > };
> > };
> >
> > partitions {
> > compatible = "fixed-partitions";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > partition@0 {
> > reg = <0x0 0x20000>;
> >
> > nvmem-cells {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@0 {
> > reg = <0x0 0x10>;
> > };
> > };
> > };
> > };
> > };
>
> If there'll be an agreement on the bindings: will you be willing to
> merge Alban's patch even without support in the code for the above
> (with the assumption that it will be added later)?
No, because Alban's patch actually allows people to define and
reference nvmem cells in a DT, but without documenting it (see my first
reply).
> My use-case is on
> non-DT systems and creating nvmem devices corresponding to MTD
> partitions if fine by me.
What you propose is option #1 in my list of proposals, and it requires
some changes to avoid automatically assigning nvmem->dev.of_node to
parent->of_node (which will be != NULL when the MTD device has been
instantiated from a DT node).
> I also don't have the means to test the
> support for these bindings if I were to actually write them myself.
And that's the very reason I proposed #1. I don't want to block this
stuff, but in its current state, I'm not willing to accept it either.
Either we agree on the binding and patch the nvmem framework to support
this new binding, or we find a way to hide the fact that the mtd
device (the nvmem parent) has a DT node attached to it.
^ permalink raw reply
* RE: [PATCH V2 net-next 2/6] sctp: Handle sctp packets with CHECKSUM_PARTIAL
From: David Laight @ 2018-08-20 15:39 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', Vladislav Yasevich
Cc: netdev@vger.kernel.org, linux-sctp@vger.kernel.org,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org, mst@redhat.com,
jasowang@redhat.com, nhorman@tuxdriver.com, Vladislav Yasevich
In-Reply-To: <20180820145415.GA5310@localhost.localdomain>
From: Marcelo Ricardo Leitner
> Sent: 20 August 2018 15:54
> On Wed, May 02, 2018 at 11:38:24AM -0300, Marcelo Ricardo Leitner wrote:
> > On Tue, May 01, 2018 at 10:07:35PM -0400, Vladislav Yasevich wrote:
> > > With SCTP checksum offload available in virtio, it is now
> > > possible for virtio to receive a sctp packet with CHECKSUM_PARTIAL
> > > set (guest-to-guest traffic). SCTP doesn't really have a
> > > partial checksum like TCP does, because CRC32c can't do partial
> > > additive checksumming.
...
Actually that isn't entirely true.
For all crc, crc(a) ^ crc(b) == crc(a^b).
Since crc(0) == 0 you can xor together two separately calculated crc
provided they both end at the same point.
The slight problem is that you are more likely to be appending
one buffer to another - which requires appending lots of zero
bytes to one of the crcs.
This could be speeded up by using lookup tables that add moderate
sized blocks of zero bytes to a crc instead of adding the zero
bytes one at a time.
Doing it without large const data and/or data cache trashing
is left as an exercise to the implementer.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API
From: Bartosz Golaszewski @ 2018-08-20 18:50 UTC (permalink / raw)
To: Boris Brezillon
Cc: Srinivas Kandagatla, Alban, Jonathan Corbet, Sekhar Nori,
Kevin Hilman, Russell King, Arnd Bergmann, Greg Kroah-Hartman,
David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
Grygorii Strashko, David S . Miller, Naren, Mauro Carvalho Chehab,
Andrew Morton, Lukas Wunner, Dan Carpenter, Florian
In-Reply-To: <20180820202038.5d3dc195@bbrezillon>
2018-08-20 20:20 GMT+02:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Mon, 20 Aug 2018 11:43:34 +0100
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
>
>>
>> Overall am still not able to clear visualize on how MTD bindings with
>> nvmem cells would look in both partition and un-partition usecases?
>> An example DT would be nice here!!
>
> Something along those lines:
>
> mtdnode {
> nvmem-cells {
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x14>;
> };
> };
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> reg = <0x0 0x20000>;
>
> nvmem-cells {
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@0 {
> reg = <0x0 0x10>;
> };
> };
> };
> };
> };
If there'll be an agreement on the bindings: will you be willing to
merge Alban's patch even without support in the code for the above
(with the assumption that it will be added later)? My use-case is on
non-DT systems and creating nvmem devices corresponding to MTD
partitions if fine by me. I also don't have the means to test the
support for these bindings if I were to actually write them myself.
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* Re: [PATCH] sunrpc: Don't use stack buffer with scatterlist
From: J. Bruce Fields @ 2018-08-20 18:50 UTC (permalink / raw)
To: Laura Abbott
Cc: Jeff Layton, Trond Myklebust, Anna Schumaker, David S. Miller,
linux-nfs, netdev, linux-kernel
In-Reply-To: <20180817214354.5780-1-labbott@redhat.com>
On Fri, Aug 17, 2018 at 02:43:54PM -0700, Laura Abbott wrote:
> Fedora got a bug report from NFS:
Thanks, applying (unless Trond or Anna gets it).
--b.
>
> kernel BUG at include/linux/scatterlist.h:143!
> ...
> RIP: 0010:sg_init_one+0x7d/0x90
> ..
> make_checksum+0x4e7/0x760 [rpcsec_gss_krb5]
> gss_get_mic_kerberos+0x26e/0x310 [rpcsec_gss_krb5]
> gss_marshal+0x126/0x1a0 [auth_rpcgss]
> ? __local_bh_enable_ip+0x80/0xe0
> ? call_transmit_status+0x1d0/0x1d0 [sunrpc]
> call_transmit+0x137/0x230 [sunrpc]
> __rpc_execute+0x9b/0x490 [sunrpc]
> rpc_run_task+0x119/0x150 [sunrpc]
> nfs4_run_exchange_id+0x1bd/0x250 [nfsv4]
> _nfs4_proc_exchange_id+0x2d/0x490 [nfsv4]
> nfs41_discover_server_trunking+0x1c/0xa0 [nfsv4]
> nfs4_discover_server_trunking+0x80/0x270 [nfsv4]
> nfs4_init_client+0x16e/0x240 [nfsv4]
> ? nfs_get_client+0x4c9/0x5d0 [nfs]
> ? _raw_spin_unlock+0x24/0x30
> ? nfs_get_client+0x4c9/0x5d0 [nfs]
> nfs4_set_client+0xb2/0x100 [nfsv4]
> nfs4_create_server+0xff/0x290 [nfsv4]
> nfs4_remote_mount+0x28/0x50 [nfsv4]
> mount_fs+0x3b/0x16a
> vfs_kern_mount.part.35+0x54/0x160
> nfs_do_root_mount+0x7f/0xc0 [nfsv4]
> nfs4_try_mount+0x43/0x70 [nfsv4]
> ? get_nfs_version+0x21/0x80 [nfs]
> nfs_fs_mount+0x789/0xbf0 [nfs]
> ? pcpu_alloc+0x6ca/0x7e0
> ? nfs_clone_super+0x70/0x70 [nfs]
> ? nfs_parse_mount_options+0xb40/0xb40 [nfs]
> mount_fs+0x3b/0x16a
> vfs_kern_mount.part.35+0x54/0x160
> do_mount+0x1fd/0xd50
> ksys_mount+0xba/0xd0
> __x64_sys_mount+0x21/0x30
> do_syscall_64+0x60/0x1f0
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> This is BUG_ON(!virt_addr_valid(buf)) triggered by using a stack
> allocated buffer with a scatterlist. Convert the buffer for
> rc4salt to be dynamically allocated instead.
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1615258
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Compile tested only.
> ---
> net/sunrpc/auth_gss/gss_krb5_crypto.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index 8654494b4d0a..834eb2b9e41b 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -169,7 +169,7 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
> struct scatterlist sg[1];
> int err = -1;
> u8 *checksumdata;
> - u8 rc4salt[4];
> + u8 *rc4salt;
> struct crypto_ahash *md5;
> struct crypto_ahash *hmac_md5;
> struct ahash_request *req;
> @@ -183,14 +183,18 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
> return GSS_S_FAILURE;
> }
>
> + rc4salt = kmalloc_array(4, sizeof(*rc4salt), GFP_NOFS);
> + if (!rc4salt)
> + return GSS_S_FAILURE;
> +
> if (arcfour_hmac_md5_usage_to_salt(usage, rc4salt)) {
> dprintk("%s: invalid usage value %u\n", __func__, usage);
> - return GSS_S_FAILURE;
> + goto out_free_rc4salt;
> }
>
> checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_NOFS);
> if (!checksumdata)
> - return GSS_S_FAILURE;
> + goto out_free_rc4salt;
>
> md5 = crypto_alloc_ahash("md5", 0, CRYPTO_ALG_ASYNC);
> if (IS_ERR(md5))
> @@ -258,6 +262,8 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
> crypto_free_ahash(md5);
> out_free_cksum:
> kfree(checksumdata);
> +out_free_rc4salt:
> + kfree(rc4salt);
> return err ? GSS_S_FAILURE : 0;
> }
>
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH] r8169: don't use MSI-X on RTL8106e
From: Bjorn Helgaas @ 2018-08-20 18:44 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, jian-hong, nic_swsd, netdev, linux-kernel, linux,
linux-pci, Marc Zyngier, Thomas Gleixner, Christoph Hellwig
In-Reply-To: <ac30d4e6-6411-09e6-1205-96e3e8e29f6f@gmail.com>
[+cc Marc, Thomas, Christoph, linux-pci)
(beginning of thread at [1])
On Thu, Aug 16, 2018 at 09:50:48PM +0200, Heiner Kallweit wrote:
> On 16.08.2018 21:39, David Miller wrote:
> > From: Heiner Kallweit <hkallweit1@gmail.com>
> > Date: Thu, 16 Aug 2018 21:37:31 +0200
> >
> >> On 16.08.2018 21:21, David Miller wrote:
> >>> From: <jian-hong@endlessm.com>
> >>> Date: Wed, 15 Aug 2018 14:21:10 +0800
> >>>
> >>>> Found the ethernet network on ASUS X441UAR doesn't come back on resume
> >>>> from suspend when using MSI-X. The chip is RTL8106e - version 39.
> >>>
> >>> Heiner, please take a look at this.
> >>>
> >>> You recently disabled MSI-X on RTL8168g for similar reasons.
> >>>
> >>> Now that we've seen two chips like this, maybe there is some other
> >>> problem afoot.
> >>>
> >> Thanks for the hint. I saw it already and just contacted Realtek
> >> whether they are aware of any MSI-X issues with particular chip
> >> versions. With the chip versions I have access to MSI-X works fine.
> >>
> >> There's also the theoretical option that the issues are caused by
> >> broken BIOS's. But so far only chip versions have been reported
> >> which are very similar, at least with regard to version number
> >> (2x VER_40, 1x VER_39). So they may share some buggy component.
> >>
> >> Let's see whether Realtek can provide some hint.
> >> If more chip versions are reported having problems with MSI-X,
> >> then we could switch to a whitelist or disable MSI-X in general.
> >
> > It could be that we need to reprogram some register(s) on resume,
> > which normally might not be needed, and that is what is causing the
> > problem with some chips.
> >
> Indeed. That's what I'm checking with Realtek.
> In the register list in the r8169 driver there's one entry which
> seems to indicate that there are MSI-X specific settings.
> However this register isn't used, and the r8168 vendor driver
> uses only MSI. And there are no public datasheets.
Do we have any information about these chip versions in other systems?
Or other devices using MSI-X in the same ASUS system? It seems
possible that there's some PCI core or suspend/resume issue with MSI-X
and this patch just avoids it without fixing the root cause.
It might be useful to have a kernel.org bugzilla with the complete
dmesg, "sudo lspci -vv" output, and /proc/interrupts contents archived
for future reference.
[1] https://lkml.kernel.org/r/20180815062110.16155-1-jian-hong@endlessm.com
^ permalink raw reply
* [PATCH v3 net-next] dt-bindings: can: rcar_can: document r8a77965 support
From: Eugeniu Rosca @ 2018-08-20 14:49 UTC (permalink / raw)
To: Simon Horman, Kieran Bingham, Sergei Shtylyov,
Wolfgang Grandegger, Marc Kleine-Budde, David S . Miller,
Rob Herring, Mark Rutland, linux-can, netdev, devicetree,
Magnus Damm, linux-renesas-soc
Cc: Eugeniu Rosca, Eugeniu Rosca
From: Eugeniu Rosca <rosca.eugeniu@gmail.com>
Document the support for rcar_can on R8A77965 SoC devices.
Add R8A77965 to the list of SoCs which require the "assigned-clocks" and
"assigned-clock-rates" properties (thanks, Sergei).
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes in v3:
- [Simon Horman] Added more recipients.
- [Simon Horman] Dropped redundant "can" word in summary line.
- [Simon Horman] Added Reviewed-by.
- [Kieran Bingham] Added Reviewed-by.
Changes in v2:
- [Kieran Bingham] Simplified commit description. Rewrapped text.
- [Sergei Shtylyov] Replaced footnotes with inline text.
- Pushed all dt-bindings patches to the beginning of the series.
---
Documentation/devicetree/bindings/net/can/rcar_can.txt | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
index 94a7f33ac5e9..60daa878c9a2 100644
--- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
+++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
@@ -13,6 +13,7 @@ Required properties:
"renesas,can-r8a7794" if CAN controller is a part of R8A7794 SoC.
"renesas,can-r8a7795" if CAN controller is a part of R8A7795 SoC.
"renesas,can-r8a7796" if CAN controller is a part of R8A7796 SoC.
+ "renesas,can-r8a77965" if CAN controller is a part of R8A77965 SoC.
"renesas,rcar-gen1-can" for a generic R-Car Gen1 compatible device.
"renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
compatible device.
@@ -28,11 +29,10 @@ Required properties:
- pinctrl-0: pin control group to be used for this controller.
- pinctrl-names: must be "default".
-Required properties for "renesas,can-r8a7795" and "renesas,can-r8a7796"
-compatible:
-In R8A7795 and R8A7796 SoCs, "clkp2" can be CANFD clock. This is a div6 clock
-and can be used by both CAN and CAN FD controller at the same time. It needs to
-be scaled to maximum frequency if any of these controllers use it. This is done
+Required properties for R8A7795, R8A7796 and R8A77965:
+For the denoted SoCs, "clkp2" can be CANFD clock. This is a div6 clock and can
+be used by both CAN and CAN FD controller at the same time. It needs to be
+scaled to maximum frequency if any of these controllers use it. This is done
using the below properties:
- assigned-clocks: phandle of clkp2(CANFD) clock.
--
2.18.0
^ permalink raw reply related
* [PATCH] net: macb: do not disable MDIO bus at open/close time
From: Anssi Hannula @ 2018-08-20 14:55 UTC (permalink / raw)
To: netdev, Nicolas Ferre, Claudiu Beznea; +Cc: David S. Miller, Andrew Lunn
In-Reply-To: <3e55fc51-503c-d174-841c-a1a5a0f5ae69@microchip.com>
macb_reset_hw() is called from macb_close() and indirectly from
macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
(Management Port Enable) bit.
This will prevent accessing any other PHYs for other Ethernet MACs on
the MDIO bus, which remains registered at macb_reset_hw() time, until
macb_init_hw() is called from macb_open() which sets the MPE bit again.
I.e. currently the MDIO bus has a short disruption at open time and is
disabled at close time until the interface is opened again.
Fix that by only touching the RE and TE bits when enabling and disabling
RX/TX.
Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
Claudiu Beznea wrote:
> On 10.08.2018 09:22, Anssi Hannula wrote:
>>
>> macb_reset_hw() is called in init path too,
>
> I only see it in macb_close() and macb_open() called from macb_init_hw().
Yeah, macb_init_hw() is what I meant :)
drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index dc09f9a8a49b..6501e9b3785a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2028,14 +2028,17 @@ static void macb_reset_hw(struct macb *bp)
{
struct macb_queue *queue;
unsigned int q;
+ u32 ctrl = macb_readl(bp, NCR);
/* Disable RX and TX (XXX: Should we halt the transmission
* more gracefully?)
*/
- macb_writel(bp, NCR, 0);
+ ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
/* Clear the stats registers (XXX: Update stats first?) */
- macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
+ ctrl |= MACB_BIT(CLRSTAT);
+
+ macb_writel(bp, NCR, ctrl);
/* Clear all status flags */
macb_writel(bp, TSR, -1);
@@ -2170,6 +2173,7 @@ static void macb_init_hw(struct macb *bp)
unsigned int q;
u32 config;
+ u32 ctrl;
macb_reset_hw(bp);
macb_set_hwaddr(bp);
@@ -2223,7 +2227,9 @@ static void macb_init_hw(struct macb *bp)
}
/* Enable TX and RX */
- macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
+ ctrl = macb_readl(bp, NCR);
+ ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
+ macb_writel(bp, NCR, ctrl);
}
/* The hash address register is 64 bits long and takes up two
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V2 net-next 2/6] sctp: Handle sctp packets with CHECKSUM_PARTIAL
From: Marcelo Ricardo Leitner @ 2018-08-20 14:54 UTC (permalink / raw)
To: Vladislav Yasevich
Cc: netdev, linux-sctp, virtualization, virtio-dev, mst, jasowang,
nhorman, Vladislav Yasevich
In-Reply-To: <20180502143824.GF5105@localhost.localdomain>
On Wed, May 02, 2018 at 11:38:24AM -0300, Marcelo Ricardo Leitner wrote:
> On Tue, May 01, 2018 at 10:07:35PM -0400, Vladislav Yasevich wrote:
> > With SCTP checksum offload available in virtio, it is now
> > possible for virtio to receive a sctp packet with CHECKSUM_PARTIAL
> > set (guest-to-guest traffic). SCTP doesn't really have a
> > partial checksum like TCP does, because CRC32c can't do partial
> > additive checksumming. It's all or nothing. So an SCTP packet
> > with CHECKSUM_PARTIAL will have checksum set to 0. Let SCTP
> > treat this as a valid checksum if CHECKSUM_PARTIAL is set.
> >
> > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> > ---
> > net/sctp/input.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > index ba8a6e6..055b8ffa 100644
> > --- a/net/sctp/input.c
> > +++ b/net/sctp/input.c
> > @@ -80,8 +80,17 @@ static inline int sctp_rcv_checksum(struct net *net, struct sk_buff *skb)
> > {
> > struct sctphdr *sh = sctp_hdr(skb);
> > __le32 cmp = sh->checksum;
> > - __le32 val = sctp_compute_cksum(skb, 0);
> > + __le32 val = 0;
> >
> > + /* In sctp PARTIAL checksum is always 0. This is a case of
> > + * a packet received from guest that supports checksum offload.
> > + * Assume it's correct as there is really no way to verify,
> > + * and we want to avaoid computing it unnecesarily.
> nit, typos --^ --^
Hi Vlad. Seems this patchset got stuck because of these.
The only other subthread that is left is actually about SCTP GSO,
which is not directly related to this.
Marcelo
^ permalink raw reply
* Re: [bpf-next RFC 1/3] flow_dissector: implements flow dissector BPF hook
From: Daniel Borkmann @ 2018-08-20 14:20 UTC (permalink / raw)
To: Willem de Bruijn, liu.song.a23
Cc: Petar Penkov, Petar Penkov, Network Development, David Miller,
Alexei Starovoitov, simon.horman, Willem de Bruijn
In-Reply-To: <CAF=yD-KciJUH6Mi_oE2rqfOPWTLvEAdinos64fL=0+dEA=_gFQ@mail.gmail.com>
On 08/20/2018 04:13 PM, Willem de Bruijn wrote:
> On Mon, Aug 20, 2018 at 1:47 AM Song Liu <liu.song.a23@gmail.com> wrote:
>> On Thu, Aug 16, 2018 at 4:14 PM, Petar Penkov <ppenkov@google.com> wrote:
>>> On Thu, Aug 16, 2018 at 3:40 PM, Song Liu <liu.song.a23@gmail.com> wrote:
>>>> On Thu, Aug 16, 2018 at 9:44 AM, Petar Penkov <peterpenkov96@gmail.com> wrote:
>>>>> From: Petar Penkov <ppenkov@google.com>
>>>>>
>>>>> Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
>>>>> attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
>>>>> path. The BPF program is kept as a global variable so it is
>>>>> accessible to all flow dissectors.
>>>>>
>>>>> Signed-off-by: Petar Penkov <ppenkov@google.com>
>>>>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>
>>>>> @@ -658,6 +698,42 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>>> FLOW_DISSECTOR_KEY_BASIC,
>>>>> target_container);
>>>>>
>>>>> + rcu_read_lock();
>>>>> + attached = rcu_dereference(flow_dissector_prog);
>>>>> + if (attached) {
>>>>> + /* Note that even though the const qualifier is discarded
>>>>> + * throughout the execution of the BPF program, all changes(the
>>>>> + * control block) are reverted after the BPF program returns.
>>>>> + * Therefore, __skb_flow_dissect does not alter the skb.
>>>>> + */
>>>>> + struct bpf_flow_dissect_cb *cb;
>>>>> + u8 cb_saved[BPF_SKB_CB_LEN];
>>>>> + u32 result;
>>>>> +
>>>>> + cb = (struct bpf_flow_dissect_cb *)(bpf_skb_cb((struct sk_buff *)skb));
>>>>> +
>>>>> + /* Save Control Block */
>>>>> + memcpy(cb_saved, cb, sizeof(cb_saved));
>>>>> + memset(cb, 0, sizeof(cb_saved));
>>>>> +
>>>>> + /* Pass parameters to the BPF program */
>>>>> + cb->nhoff = nhoff;
>>>>> + cb->target_container = target_container;
>>>>> + cb->flow_dissector = flow_dissector;
>>>>> +
>>>>> + bpf_compute_data_pointers((struct sk_buff *)skb);
>>>>> + result = BPF_PROG_RUN(attached, skb);
>>>>> +
>>>>> + /* Restore state */
>>>>> + memcpy(cb, cb_saved, sizeof(cb_saved));
>>>>> +
>>>>> + key_control->thoff = min_t(u16, key_control->thoff,
>>>>> + skb ? skb->len : hlen);
>>>>> + rcu_read_unlock();
>>>>> + return result == BPF_OK;
>>>>> + }
>>>>
>>>> If the BPF program cannot handle certain protocol, shall we fall back
>>>> to the built-in logic? Otherwise, all BPF programs need to have some
>>>> code for all protocols.
>>>>
>>>> Song
>>>
>>> I believe that if we fall back to the built-in logic we lose all security
>>> guarantees from BPF and this is why the code does not support
>>> fall back.
>>>
>>> Petar
>>
>> I am not really sure we are on the same page. I am proposing 3
>> different return values from BPF_PROG_RUN(), and they should be
>> handled as
>>
>> 1. result == BPF_OK => return true;
>> 2. result == BPF_DROP => return false;
>> 3. result == something else => fall back.
>>
>> Does this proposal make any sense?
>>
>> Thanks,
>> Song
>
> It certainly makes sense. We debated it initially, as well.
>
> In the short term, it allows for simpler BPF programs, as they can
> off-load some protocols to the C implementation.
>
> But the RFC patchset already implements most protocols in BPF.
> I had not expected that when we started out.
>
> Eventually, I think it is preferable to just deprecate the C
> implementation. Which is not possible if we make this opt-out
> a part of the BPF flow dissector interface.
+1
> There is also the lesser issue that a buggy BPF program might
> accidentally pass the third value and unknowing open itself up
> to the large attack surface. Without this option, the security
> audit is much simpler.
Fully agree, I'm all for dropping such option.
Thanks,
Daniel
^ permalink raw reply
* Re: [bpf-next RFC 1/3] flow_dissector: implements flow dissector BPF hook
From: Willem de Bruijn @ 2018-08-20 14:13 UTC (permalink / raw)
To: liu.song.a23
Cc: Petar Penkov, Petar Penkov, Network Development, David Miller,
Alexei Starovoitov, Daniel Borkmann, simon.horman,
Willem de Bruijn
In-Reply-To: <CAPhsuW6w7OX4vjXhA=rH51rBZd-gjtvckUseZP77P5fez5As2A@mail.gmail.com>
On Mon, Aug 20, 2018 at 1:47 AM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Thu, Aug 16, 2018 at 4:14 PM, Petar Penkov <ppenkov@google.com> wrote:
> > On Thu, Aug 16, 2018 at 3:40 PM, Song Liu <liu.song.a23@gmail.com> wrote:
> >>
> >> On Thu, Aug 16, 2018 at 9:44 AM, Petar Penkov <peterpenkov96@gmail.com> wrote:
> >> > From: Petar Penkov <ppenkov@google.com>
> >> >
> >> > Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
> >> > attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
> >> > path. The BPF program is kept as a global variable so it is
> >> > accessible to all flow dissectors.
> >> >
> >> > Signed-off-by: Petar Penkov <ppenkov@google.com>
> >> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> >> > @@ -658,6 +698,42 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >> > FLOW_DISSECTOR_KEY_BASIC,
> >> > target_container);
> >> >
> >> > + rcu_read_lock();
> >> > + attached = rcu_dereference(flow_dissector_prog);
> >> > + if (attached) {
> >> > + /* Note that even though the const qualifier is discarded
> >> > + * throughout the execution of the BPF program, all changes(the
> >> > + * control block) are reverted after the BPF program returns.
> >> > + * Therefore, __skb_flow_dissect does not alter the skb.
> >> > + */
> >> > + struct bpf_flow_dissect_cb *cb;
> >> > + u8 cb_saved[BPF_SKB_CB_LEN];
> >> > + u32 result;
> >> > +
> >> > + cb = (struct bpf_flow_dissect_cb *)(bpf_skb_cb((struct sk_buff *)skb));
> >> > +
> >> > + /* Save Control Block */
> >> > + memcpy(cb_saved, cb, sizeof(cb_saved));
> >> > + memset(cb, 0, sizeof(cb_saved));
> >> > +
> >> > + /* Pass parameters to the BPF program */
> >> > + cb->nhoff = nhoff;
> >> > + cb->target_container = target_container;
> >> > + cb->flow_dissector = flow_dissector;
> >> > +
> >> > + bpf_compute_data_pointers((struct sk_buff *)skb);
> >> > + result = BPF_PROG_RUN(attached, skb);
> >> > +
> >> > + /* Restore state */
> >> > + memcpy(cb, cb_saved, sizeof(cb_saved));
> >> > +
> >> > + key_control->thoff = min_t(u16, key_control->thoff,
> >> > + skb ? skb->len : hlen);
> >> > + rcu_read_unlock();
> >> > + return result == BPF_OK;
> >> > + }
> >>
> >> If the BPF program cannot handle certain protocol, shall we fall back
> >> to the built-in logic? Otherwise, all BPF programs need to have some
> >> code for all protocols.
> >>
> >> Song
> >
> > I believe that if we fall back to the built-in logic we lose all security
> > guarantees from BPF and this is why the code does not support
> > fall back.
> >
> > Petar
>
> I am not really sure we are on the same page. I am proposing 3
> different return values from BPF_PROG_RUN(), and they should be
> handled as
>
> 1. result == BPF_OK => return true;
> 2. result == BPF_DROP => return false;
> 3. result == something else => fall back.
>
> Does this proposal make any sense?
>
> Thanks,
> Song
It certainly makes sense. We debated it initially, as well.
In the short term, it allows for simpler BPF programs, as they can
off-load some protocols to the C implementation.
But the RFC patchset already implements most protocols in BPF.
I had not expected that when we started out.
Eventually, I think it is preferable to just deprecate the C
implementation. Which is not possible if we make this opt-out
a part of the BPF flow dissector interface.
There is also the lesser issue that a buggy BPF program might
accidentally pass the third value and unknowing open itself up
to the large attack surface. Without this option, the security
audit is much simpler.
^ permalink raw reply
* Re: [PATCH 0/2] net/sched: Add hardware specific counters to TC actions
From: Eelco Chaudron @ 2018-08-20 14:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, netdev, jhs, xiyou.wangcong, jiri, simon.horman,
Marcelo Ricardo Leitner
In-Reply-To: <20180817042722.0e534ce0@cakuba>
On 17 Aug 2018, at 13:27, Jakub Kicinski wrote:
> On Thu, 16 Aug 2018 14:02:44 +0200, Eelco Chaudron wrote:
>> On 11 Aug 2018, at 21:06, David Miller wrote:
>>
>>> From: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> Date: Thu, 9 Aug 2018 20:26:08 -0700
>>>
>>>> It is not immediately clear why this is needed. The memory and
>>>> updating two sets of counters won't come for free, so perhaps a
>>>> stronger justification than troubleshooting is due? :S
>>>>
>>>> Netdev has counters for fallback vs forwarded traffic, so you'd
>>>> know
>>>> that traffic hits the SW datapath, plus the rules which are in_hw
>>>> will
>>>> most likely not match as of today for flower (assuming
>>>> correctness).
>>
>> I strongly believe that these counters are a requirement for a mixed
>> software/hardware (flow) based forwarding environment. The global
>> counters will not help much here as you might have chosen to have
>> certain traffic forwarded by software.
>>
>> These counters are probably the only option you have to figure out
>> why
>> forwarding is not as fast as expected, and you want to blame the TC
>> offload NIC.
>
> The suggested debugging flow would be:
> (1) check the global counter for fallback are incrementing;
> (2) find a flow with high stats but no in_hw flag set.
>
> The in_hw indication should be sufficient in most cases (unless there
> are shared blocks between netdevs of different ASICs...).
I guess the aim is to find miss behaving hardware, i.e. having the in_hw
flag set, but flows still coming to the kernel.
>>>> I'm slightly concerned about potential performance impact, would
>>>> you
>>>> be able to share some numbers for non-trivial number of flows (100k
>>>> active?)?
>>>
>>> Agreed, features used for diagnostics cannot have a harmful penalty
>>> for fast path performance.
>>
>> Fast path performance is not affected as these counters are not
>> incremented there. They are only incremented by the nic driver when
>> they
>> gather their statistics from hardware.
>
> Not by much, you are adding state to performance-critical structures,
> though, for what is effectively debugging purposes.
>
> I was mostly talking about the HW offload stat updates (sorry for not
> being clear).
>
> We can have some hundreds of thousands active offloaded flows, each of
> them can have multiple actions, and stats have to be updated multiple
> times per second and dumped probably around once a second, too. On a
> busy system the stats will get evicted from cache between each round.
>
> But I'm speculating let's see if I can get some numbers on it (if you
> could get some too, that would be great!).
I’ll try to measure some of this later this week/early next week.
>> However, the flow creation is effected, as this is where the extra
>> memory gets allocated. I had done some 40K flow tests before and did
>> not
>> see any noticeable change in flow insertion performance. As requested
>> by
>> Jakub I did it again for 100K (and threw a Netronome blade in the mix
>> ;). I used Marcelo’s test tool,
>> https://github.com/marceloleitner/perf-flower.git.
>>
>> Here are the numbers (time in seconds) for 10 runs in sorted order:
>>
>> +-------------+----------------+
>> | Base_kernel | Change_applied |
>> +-------------+----------------+
>> | 5.684019 | 5.656388 |
>> | 5.699658 | 5.674974 |
>> | 5.725220 | 5.722107 |
>> | 5.739285 | 5.839855 |
>> | 5.748088 | 5.865238 |
>> | 5.766231 | 5.873913 |
>> | 5.842264 | 5.909259 |
>> | 5.902202 | 5.912685 |
>> | 5.905391 | 5.947138 |
>> | 6.032997 | 5.997779 |
>> +-------------+----------------+
>>
>> I guess the deviation is in the userspace part, which is where in
>> real
>> life flows get added anyway.
>>
>> Let me know if more is unclear.
^ permalink raw reply
* Re: [PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node
From: Andrew Lunn @ 2018-08-20 13:56 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: David S. Miller, Nicolas Ferre, kernel, netdev, mdf, Brad Mouring,
Florian Fainelli
In-Reply-To: <2c7d7e81-322d-c995-7de6-64a25906d280@pengutronix.de>
> I've done so in part 4/4.
Yes, i noticed that later. Ideally, it would be here, since this is
the patch which actually changes the binding.
Andrew
^ permalink raw reply
* Re: [PATCH 2/4] of: phy: Warn about unexpected fixed-links in of_mdiobus_register
From: Ahmad Fatoum @ 2018-08-20 13:51 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Nicolas Ferre, kernel, netdev, mdf, Brad Mouring,
Florian Fainelli
In-Reply-To: <20180820133734.GB6583@lunn.ch>
On 08/20/2018 03:37 PM, Andrew Lunn wrote:
> We should be more specific than "device tree". It is actually the "mdio bus subtree".
I wasn't sure if there are more drivers that call of_mdiobus_register for the MAC node.
I'll update the message.
> Is this patch on its own sufficient to fix your regression? Ideally we
> want one small patch which we can add to stable to fix the regression,
> and then all the new functionality goes into net-next.
This patch (or at least the fixed version in v3) only replaces the MDIO bus scan for
fixed-links with a warning. Patch 1/4 (the one Cc:ing stable) suffices to fix the regression.
^ permalink raw reply
* Re: [PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node
From: Ahmad Fatoum @ 2018-08-20 13:45 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Nicolas Ferre, kernel, netdev, mdf, Brad Mouring,
Florian Fainelli
In-Reply-To: <20180820134234.GC6583@lunn.ch>
On 08/20/2018 03:42 PM, Andrew Lunn wrote:
> On Mon, Aug 20, 2018 at 02:12:37PM +0200, Ahmad Fatoum wrote:
> This is correct. But i would prefer the more readable
>
> struct device_node *node = of_get_child_by_name(np, "mdio");
>
> if (!node)
> /* Allow for the deprecated PHYs in the MAC node. */
> node = np;
>
>> if (pdata)
>> bp->mii_bus->phy_mask = pdata->phy_mask;
>> -
>> - err = of_mdiobus_register(bp->mii_bus, np);
>> + err = of_mdiobus_register(bp->mii_bus, node);
>> }
Ok.
> Also, the device tree binding documentation needs updating.
I've done so in part 4/4.
^ permalink raw reply
* Re: [PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node
From: Andrew Lunn @ 2018-08-20 13:42 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: David S. Miller, Nicolas Ferre, kernel, netdev, mdf, Brad Mouring,
Florian Fainelli
In-Reply-To: <20180820121238.7779-3-a.fatoum@pengutronix.de>
On Mon, Aug 20, 2018 at 02:12:37PM +0200, Ahmad Fatoum wrote:
> To align macb DT entries with those of other MACs.
> For backwards compatibility, the old way remains supported.
>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ef6ce8691443..2ebc5698db9d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -596,10 +596,10 @@ static int macb_mii_init(struct macb *bp)
>
> err = mdiobus_register(bp->mii_bus);
> } else {
> + struct device_node *node = of_get_child_by_name(np, "mdio") ?: np;
This is correct. But i would prefer the more readable
struct device_node *node = of_get_child_by_name(np, "mdio");
if (!node)
/* Allow for the deprecated PHYs in the MAC node. */
node = np;
> if (pdata)
> bp->mii_bus->phy_mask = pdata->phy_mask;
> -
> - err = of_mdiobus_register(bp->mii_bus, np);
> + err = of_mdiobus_register(bp->mii_bus, node);
> }
Also, the device tree binding documentation needs updating.
Thanks
Andrew
^ 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