* [PATCH 1/2] b43: fix unitialized reads of ret by initializing the array to zero
From: Colin King @ 2017-09-05 18:15 UTC (permalink / raw)
To: Kalle Valo, linux-wireless, b43-dev, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The u8 char array ret is not being initialized and elements outside
the range start to end contain just garbage values from the stack.
This results in a later scan of the array to read potentially
uninitialized values. Fix this by initializing the array to zero.
This seems to have been an issue since the very first commit.
Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/broadcom/b43/phy_g.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/b43/phy_g.c b/drivers/net/wireless/broadcom/b43/phy_g.c
index 822dcaa8ace6..f59c02166462 100644
--- a/drivers/net/wireless/broadcom/b43/phy_g.c
+++ b/drivers/net/wireless/broadcom/b43/phy_g.c
@@ -2297,7 +2297,7 @@ static u8 b43_gphy_aci_detect(struct b43_wldev *dev, u8 channel)
static u8 b43_gphy_aci_scan(struct b43_wldev *dev)
{
struct b43_phy *phy = &dev->phy;
- u8 ret[13];
+ u8 ret[13] = { 0 };
unsigned int channel = phy->channel;
unsigned int i, j, start, end;
--
2.14.1
^ permalink raw reply related
* [PATCH 1/3] drivers: net: xgene: Read tx/rx delay for ACPI
From: Iyappan Subramanian @ 2017-09-05 18:16 UTC (permalink / raw)
To: davem, netdev
Cc: Quan Nguyen, dnelson, patches, linux-arm-kernel,
Iyappan Subramanian
In-Reply-To: <1504635392-19089-1-git-send-email-isubramanian@apm.com>
This patch fixes reading tx/rx delay values for ACPI.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 6e253d9..655c0fc 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -1591,7 +1591,7 @@ static int xgene_get_tx_delay(struct xgene_enet_pdata *pdata)
struct device *dev = &pdata->pdev->dev;
int delay, ret;
- ret = of_property_read_u32(dev->of_node, "tx-delay", &delay);
+ ret = device_property_read_u32(dev, "tx-delay", &delay);
if (ret) {
pdata->tx_delay = 4;
return 0;
@@ -1612,7 +1612,7 @@ static int xgene_get_rx_delay(struct xgene_enet_pdata *pdata)
struct device *dev = &pdata->pdev->dev;
int delay, ret;
- ret = of_property_read_u32(dev->of_node, "rx-delay", &delay);
+ ret = device_property_read_u32(dev, "rx-delay", &delay);
if (ret) {
pdata->rx_delay = 2;
return 0;
--
2.7.4
^ permalink raw reply related
* [PATCH 0/3] drivers: net: xgene: Misc bug fixes
From: Iyappan Subramanian @ 2017-09-05 18:16 UTC (permalink / raw)
To: davem, netdev; +Cc: linux-arm-kernel, dnelson, patches, Iyappan Subramanian
This patch set fixes bugs related to handling the case for ACPI for,
reading and programming tx/rx delay values.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
---
Iyappan Subramanian (2):
drivers: net: xgene: Read tx/rx delay for ACPI
drivers: net: xgene: Remove return statement from void function
Quan Nguyen (1):
drivers: net: xgene: Configure tx/rx delay for ACPI
drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 7 ++-----
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 6 ++----
2 files changed, 4 insertions(+), 9 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH 2/3] drivers: net: xgene: Configure tx/rx delay for ACPI
From: Iyappan Subramanian @ 2017-09-05 18:16 UTC (permalink / raw)
To: davem, netdev
Cc: linux-arm-kernel, dnelson, patches, Quan Nguyen,
Iyappan Subramanian
In-Reply-To: <1504635392-19089-1-git-send-email-isubramanian@apm.com>
From: Quan Nguyen <qnguyen@apm.com>
This patch fixes configuring tx/rx delay values for ACPI.
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
---
drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
index e45b587..3188f55 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
@@ -468,7 +468,6 @@ static void xgene_enet_configure_clock(struct xgene_enet_pdata *pdata)
static void xgene_gmac_set_speed(struct xgene_enet_pdata *pdata)
{
- struct device *dev = &pdata->pdev->dev;
u32 icm0, icm2, mc2;
u32 intf_ctl, rgmii, value;
@@ -500,10 +499,8 @@ static void xgene_gmac_set_speed(struct xgene_enet_pdata *pdata)
intf_ctl |= ENET_GHD_MODE;
CFG_MACMODE_SET(&icm0, 2);
CFG_WAITASYNCRD_SET(&icm2, 0);
- if (dev->of_node) {
- CFG_TXCLK_MUXSEL0_SET(&rgmii, pdata->tx_delay);
- CFG_RXCLK_MUXSEL0_SET(&rgmii, pdata->rx_delay);
- }
+ CFG_TXCLK_MUXSEL0_SET(&rgmii, pdata->tx_delay);
+ CFG_RXCLK_MUXSEL0_SET(&rgmii, pdata->rx_delay);
rgmii |= CFG_SPEED_1250;
xgene_enet_rd_csr(pdata, DEBUG_REG_ADDR, &value);
--
2.7.4
^ permalink raw reply related
* [PATCH 3/3] drivers: net: xgene: Remove return statement from void function
From: Iyappan Subramanian @ 2017-09-05 18:16 UTC (permalink / raw)
To: davem, netdev; +Cc: linux-arm-kernel, dnelson, patches, Iyappan Subramanian
In-Reply-To: <1504635392-19089-1-git-send-email-isubramanian@apm.com>
commit 183db4 ("drivers: net: xgene: Correct probe sequence handling")
changed the return type of xgene_enet_check_phy_handle() to void.
This patch, removes the return statement from the last line.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
---
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 655c0fc..3b889ef 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -1674,8 +1674,6 @@ static void xgene_enet_check_phy_handle(struct xgene_enet_pdata *pdata)
ret = xgene_enet_phy_connect(pdata->ndev);
if (!ret)
pdata->mdio_driver = true;
-
- return;
}
static void xgene_enet_gpiod_get(struct xgene_enet_pdata *pdata)
--
2.7.4
^ permalink raw reply related
* [PATCH 2/2] b43legacy: fix unitialized reads of ret by initializing the array to zero
From: Colin King @ 2017-09-05 18:16 UTC (permalink / raw)
To: Larry Finger, Kalle Valo, linux-wireless, b43-dev, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The u8 char array ret is not being initialized and elements outside
the range start to end contain just garbage values from the stack.
This results in a later scan of the array to read potentially
uninitialized values. Fix this by initializing the array to zero.
This seems to have been an issue since the very first commit.
Detected by CoverityScan CID#139653 ("Uninitialized scalar variable")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/broadcom/b43legacy/radio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/b43legacy/radio.c b/drivers/net/wireless/broadcom/b43legacy/radio.c
index 9501420340a9..eab1c9387846 100644
--- a/drivers/net/wireless/broadcom/b43legacy/radio.c
+++ b/drivers/net/wireless/broadcom/b43legacy/radio.c
@@ -280,7 +280,7 @@ u8 b43legacy_radio_aci_detect(struct b43legacy_wldev *dev, u8 channel)
u8 b43legacy_radio_aci_scan(struct b43legacy_wldev *dev)
{
struct b43legacy_phy *phy = &dev->phy;
- u8 ret[13];
+ u8 ret[13] = { 0 };
unsigned int channel = phy->channel;
unsigned int i;
unsigned int j;
--
2.14.1
^ permalink raw reply related
* Re: [RFC net-next] net: sch_clsact: add support for global per-netns classifier mode
From: Cong Wang @ 2017-09-05 18:18 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Linux Kernel Network Developers, Roopa Prabhu, David Ahern,
Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <1504615701-20912-1-git-send-email-nikolay@cumulusnetworks.com>
On Tue, Sep 5, 2017 at 5:48 AM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> Hi all,
> This RFC adds a new mode for clsact which designates a device's egress
> classifier as global per netns. The packets that are not classified for
> a particular device will be classified using the global classifier.
> We have needed a global classifier for some time now for various
> purposes and setting the single bridge or loopback/vrf device as the
> global classifier device is acceptable for us. Doing it this way avoids
> the act/cls device and queue dependencies.
>
> This is strictly an RFC patch just to show the intent, if we agree on
> the details the proposed patch will have support for both ingress and
> egress, and will be using a static key to avoid the fast path test when no
> global classifier has been configured.
>
> Example (need a modified tc that adds TCA_OPTIONS when using q_clsact):
> $ tc qdisc add dev lo clsact global
> $ tc filter add dev lo egress protocol ip u32 match ip dst 4.3.2.1/32 action drop
>
> the last filter will be global for all devices that don't have a
> specific egress_cl_list (i.e. have clsact configured).
Sorry this is too ugly.
netdevice is still implied in your command line even if you treat it
as global. It is essentially hard to bypass netdevice layer since
netdevice is the core of L2 and also where everything begins.
Maybe the best we can do here is make tc filters standalone
as tc actions so that filters can exist before qdisc's and netdevices.
But this probably requires significant works to make it working
with both existing non-standalone and bindings standalones
with qdisc's.
Note I don't look deeply into this, just one thought, at least this
appears less ugly than yours.
^ permalink raw reply
* Re: [RFC net-next] net: sch_clsact: add support for global per-netns classifier mode
From: Nikolay Aleksandrov @ 2017-09-05 18:25 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Roopa Prabhu, David Ahern,
Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <CAM_iQpUYGH4BQ-ZUShbRAy6aJD2fYuM+oc_kWYyQ51n+===CQw@mail.gmail.com>
On 9/5/17 9:18 PM, Cong Wang wrote:
> On Tue, Sep 5, 2017 at 5:48 AM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com> wrote:
>> Hi all,
>> This RFC adds a new mode for clsact which designates a device's egress
>> classifier as global per netns. The packets that are not classified for
>> a particular device will be classified using the global classifier.
>> We have needed a global classifier for some time now for various
>> purposes and setting the single bridge or loopback/vrf device as the
>> global classifier device is acceptable for us. Doing it this way avoids
>> the act/cls device and queue dependencies.
>>
>> This is strictly an RFC patch just to show the intent, if we agree on
>> the details the proposed patch will have support for both ingress and
>> egress, and will be using a static key to avoid the fast path test when no
>> global classifier has been configured.
>>
>> Example (need a modified tc that adds TCA_OPTIONS when using q_clsact):
>> $ tc qdisc add dev lo clsact global
>> $ tc filter add dev lo egress protocol ip u32 match ip dst 4.3.2.1/32 action drop
>>
>> the last filter will be global for all devices that don't have a
>> specific egress_cl_list (i.e. have clsact configured).
>
> Sorry this is too ugly
>
> netdevice is still implied in your command line even if you treat it
> as global. It is essentially hard to bypass netdevice layer since
> netdevice is the core of L2 and also where everything begins.
>
This is only a quick RFC, that can be removed entirely if we limit it to the
netns and its loopback device. Then we can drop the "device" keyword altogether.
> Maybe the best we can do here is make tc filters standalone
> as tc actions so that filters can exist before qdisc's and netdevices.
> But this probably requires significant works to make it working
> with both existing non-standalone and bindings standalones
> with qdisc's.
>
We've actually been discussing this option internally as well.
I think we'll look into doing that regardless of this patch.
> Note I don't look deeply into this, just one thought, at least this
> appears less ugly than yours.
>
What I did was aimed at simplicity and is merely a mode of clsact which
doesn't have an impact if not configured. Every other solution requires
a much more invasive change, note that doesn't mean I don't agree. :-)
^ permalink raw reply
* Re: [PATCH] radix-tree: must check __radix_tree_preload() return value
From: Eric Dumazet @ 2017-09-05 18:34 UTC (permalink / raw)
To: Linus Torvalds
Cc: Matthew Wilcox, Matthew Wilcox, Kirill A. Shutemov, Andrew Morton,
netdev, linux-kernel
In-Reply-To: <CA+55aFwNK39yB_wbwKSkk4VfLSxALzeZHT6dShpT1zZdpmeq1Q@mail.gmail.com>
On Tue, 2017-09-05 at 11:07 -0700, Linus Torvalds wrote:
> On Tue, Sep 5, 2017 at 10:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > @@ -2104,7 +2104,10 @@ EXPORT_SYMBOL(radix_tree_tagged);
> > */
> > void idr_preload(gfp_t gfp_mask)
> > {
> > - __radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
> > + int ret = __radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
> > +
> > + if (ret)
> > + preempt_disable();
> > }
> > EXPORT_SYMBOL(idr_preload);
>
> Is there a reason for the "ret" variable that is entirely mis-named,
> since it's never actually used as a return value?
>
> (Sure. it's the return value of a function, but that is entirely
> useless and pointless information, and adds no value. We should name
> variables by the data they contain or how they are used, not by "it
> was the return value of a function").
>
> In other words, why isn't this just
>
> if (__radix_tree_preload(..))
> preempt_disable();
>
> which is shorter and clearer and not confusing?
...
> Same issue, but this time strengthened by an additional "why doesn't
> this just use that idr_preload function then?" question..
>
Yep, I will send a v2, thanks.
^ permalink raw reply
* Re: [PATCH v2 net-next 0/2] flow_dissector: Flow dissector fixes
From: David Miller @ 2017-09-05 18:45 UTC (permalink / raw)
To: tom; +Cc: netdev, hannes, alex.popov
In-Reply-To: <20170901210412.2915-1-tom@quantonium.net>
From: Tom Herbert <tom@quantonium.net>
Date: Fri, 1 Sep 2017 14:04:10 -0700
> This patch set fixes some basic issues with __skb_flow_dissect function.
>
> Items addressed:
> - Cleanup control flow in the function; in particular eliminate a
> bunch of goto's and implement a simplified control flow model
> - Add limits for number of encapsulations and headers that can be
> dissected
>
> v2:
> - Simplify the logic for limits on flow dissection. Just set the
> limit based on the number of headers the flow dissector can
> processes. The accounted headers includes encapsulation headers,
> extension headers, or other shim headers.
>
> Tested:
>
> Ran normal traffic, GUE, and VXLAN traffic.
Applied, thanks Tom.
^ permalink raw reply
* [PATCH v2] radix-tree: must check __radix_tree_preload() return value
From: Eric Dumazet @ 2017-09-05 18:46 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Matthew Wilcox, Kirill A. Shutemov, Andrew Morton, netdev,
Linus Torvalds, linux-kernel
In-Reply-To: <1504634367.15310.59.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
__radix_tree_preload() only disables preemption if no error is returned.
So we really need to make sure callers always check the return value.
idr_preload() contract is to always disable preemption, so we need
to add a missing preempt_disable() if an error happened.
Similarly, ida_pre_get() only needs to call preempt_enable() in the
case no error happened.
Fixes: 0a835c4f090a ("Reimplement IDR and IDA using the radix tree")
Fixes: 7ad3d4d85c7a ("ida: Move ida_bitmap to a percpu variable")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: <stable@vger.kernel.org> [4.11+]
---
v2: addressed Linus feedback, not adding useless/confusing 'ret' variables.
lib/radix-tree.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 3527eb364964..afb3cb4d44b6 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -463,7 +463,7 @@ radix_tree_node_free(struct radix_tree_node *node)
* To make use of this facility, the radix tree must be initialised without
* __GFP_DIRECT_RECLAIM being passed to INIT_RADIX_TREE().
*/
-static int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
+static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
{
struct radix_tree_preload *rtp;
struct radix_tree_node *node;
@@ -2104,7 +2104,8 @@ EXPORT_SYMBOL(radix_tree_tagged);
*/
void idr_preload(gfp_t gfp_mask)
{
- __radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
+ if (__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE))
+ preempt_disable();
}
EXPORT_SYMBOL(idr_preload);
@@ -2118,13 +2119,13 @@ EXPORT_SYMBOL(idr_preload);
*/
int ida_pre_get(struct ida *ida, gfp_t gfp)
{
- __radix_tree_preload(gfp, IDA_PRELOAD_SIZE);
/*
* The IDA API has no preload_end() equivalent. Instead,
* ida_get_new() can return -EAGAIN, prompting the caller
* to return to the ida_pre_get() step.
*/
- preempt_enable();
+ if (!__radix_tree_preload(gfp, IDA_PRELOAD_SIZE))
+ preempt_enable();
if (!this_cpu_read(ida_bitmap)) {
struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);
^ permalink raw reply related
* Re: [PATCH 6/10] ixgbe: Use ARRAY_SIZE macro
From: David Miller @ 2017-09-05 18:50 UTC (permalink / raw)
To: thomas; +Cc: netdev, linux-kernel
In-Reply-To: <1504439110050-1907815276-6-diffsplit-thomas@m3y3r.de>
From: Thomas Meyer <thomas@m3y3r.de>
Date: Sun, 03 Sep 2017 14:19:31 +0200
> Use ARRAY_SIZE macro, rather than explicitly coding some variant of it
> yourself.
> Found with: find -type f -name "*.c" -o -name "*.h" | xargs perl -p -i -e
> 's/\bsizeof\s*\(\s*(\w+)\s*\)\s*\ /\s*sizeof\s*\(\s*\1\s*\[\s*0\s*\]\s*\)
> /ARRAY_SIZE(\1)/g' and manual check/verification.
>
> Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
This should be submitted to the Intel ethernet driver maintainers.
Thank you.
^ permalink raw reply
* Re: [PATCH 7/10] net/mlx4_core: Use ARRAY_SIZE macro
From: David Miller @ 2017-09-05 18:50 UTC (permalink / raw)
To: thomas-VsYtu1Qij5c
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1504439110050-1961876957-7-diffsplit-thomas-VsYtu1Qij5c@public.gmane.org>
From: Thomas Meyer <thomas-VsYtu1Qij5c@public.gmane.org>
Date: Sun, 03 Sep 2017 14:19:31 +0200
> Use ARRAY_SIZE macro, rather than explicitly coding some variant of it
> yourself.
> Found with: find -type f -name "*.c" -o -name "*.h" | xargs perl -p -i -e
> 's/\bsizeof\s*\(\s*(\w+)\s*\)\s*\ /\s*sizeof\s*\(\s*\1\s*\[\s*0\s*\]\s*\)
> /ARRAY_SIZE(\1)/g' and manual check/verification.
>
> Signed-off-by: Thomas Meyer <thomas-VsYtu1Qij5c@public.gmane.org>
Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next] bridge: switchdev: Use an helper to clear forward mark
From: David Miller @ 2017-09-05 18:51 UTC (permalink / raw)
To: idosch; +Cc: netdev, stephen, nikolay, yotamg, bridge, mlxsw
In-Reply-To: <20170903144413.21946-1-idosch@mellanox.com>
From: Ido Schimmel <idosch@mellanox.com>
Date: Sun, 3 Sep 2017 17:44:13 +0300
> Instead of using ifdef in the C file.
>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Suggested-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Tested-by: Yotam Gigi <yotamg@mellanox.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next v2 RESEND 0/4] net: dsa: Allow switch drivers to indicate number of TX queues
From: David Miller @ 2017-09-05 18:53 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, vivien.didelot
In-Reply-To: <20170904032703.4593-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Sun, 3 Sep 2017 20:26:59 -0700
> This patch series extracts the parts of the patch set that are likely not to be
> controversial and actually bringing multi-queue support to DSA-created network
> devices.
>
> With these patches, we can now use sch_multiq as documented under
> Documentation/networking/multique.txt and let applications dedice the switch
> port output queue they want to use. Currently only Broadcom tags utilize that
> information.
>
> Resending based on David's feedback regarding the patches not in patchwork.
>
> Changes in v2:
> - use a proper define for the number of TX queues in bcm_sf2.c (Andrew)
>
> Changes from RFC:
>
> - dropped the ability to configure RX queues since we don't do anything with
> those just yet
> - dropped the patches that dealt with binding the DSA slave network devices
> queues with their master network devices queues this will be worked on
> separately.
Series applied, thanks.
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Hannes Frederic Sowa @ 2017-09-05 19:20 UTC (permalink / raw)
To: Tom Herbert
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <CALx6S345jR6UtAGju9TaPxAOMgkBG2RZ+eMWz80ix1joSmQCog@mail.gmail.com>
Hi Tom,
Tom Herbert <tom@herbertland.com> writes:
> On Tue, Sep 5, 2017 at 4:14 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Tom Herbert <tom@herbertland.com> writes:
>>
>>> There is absolutely no requirement in IP that packets are delivered in
>>> order-- there never has been and there never will be! If the ULP, like
>>> Ethernet encapsulation, requires in order deliver then it needs to
>>> implement that itself like TCP, GRE, and other protocols ensure that
>>> with sequence numbers and reassembly. All of these hoops we do make
>>> sure that packets always follow the same path and are always in order
>>> are done for benefit of middlebox devices like stateful firewalls that
>>> have force us to adopt their concept of network architecture-- in the
>>> long run this is self-defeating and kills our ability to innovate.
>>>
>>> I'm not saying that we shouldn't consider legacy devices, but we
>>> should scrutinize new development or solutions that perpetuate
>>> incorrect design or bad assumptions.
>>
>> So configure RSS per port and ensure no fragments are send to those
>> ports. This is possible and rather easy to do. It solves the problem
>> with legacy software and it spreads out packets for your applications.
>>
>> It is not perfect but it is working and solves both problems.
>>
> Hannes,
>
> I don't see how that solves anything. The purpose of RSS is to
> distribute the load of protocol packets across queues. This needs to
> work for UDP applications. For instance, if I were building a QUIC
> server I'd want the sort of flow distribution that a TCP server would
> give. You can't do that by configuring a few ports in the device.
I seriously am with you and I think you are on the right track. I would
also much rather like to see fragmentation *not* happening and would
like to see hardware logic *not* parsing deep down into packets and
protocols. We can agree on that!
Albeit I don't understand the problem with my approach:
If you know that you are running a QUIC server and know your environment
(safe against reordering), run `ethtool -N ethX rx-flow-hash udp4 sdfn'
on the box and you would get spreading of UDP packets based on the UDP
src+dst port numbers to the rx queues. Otherwise only source and
destination addresses are being considered, which is the safe default
(maybe this is sometimes enough).
Intel network cards warn you if you actually switch the mode:
-- >8 --
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c: "enabling UDP RSS: fragmented packets may arrive out of order to the stack above\n");
drivers/net/ethernet/intel/igb/igb_ethtool.c: "enabling UDP RSS: fragmented packets may arrive out of order to the stack above\n");
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c: e_warn(drv, "enabling UDP RSS: fragmented packets may arrive out of order to the stack above\n");
-- 8< --
I argue that we can't change the default.
Furthermore I tried to argue that in a cloud where you don't know which
applications you run within the encapsulated networks, it is very hard
to predict which protocols can be considered safe against those
effects. I agree, that most of the time, it probably should not
matter. But given the rule about kernel backwards compatibility I do
have doubts that this change would go unnoticed.
I am the devil's advocate here and argue it is on us to show that is
safe before we change the semantics.
> If I were to suggest any HW change it would be to not do DPI on
> fragments (MF or offset is set). This ensures that all packets of the
> fragment train get hashed to the same queue and is on fact what RPS
> has been doing for years without any complaints.
The same UDP flow could consist out of fragments and non-fragmented
packets. Even if hardware would act according to what you wrote,
reordering is very much possible within the same UDP flow.
In case of QUIC, as far as I know, it uses destination ports 80 and
443. Like we already notify NICs about vxlan and geneve port numbers, we
could selectively enable RSS for UDP ports on those destination port
numbers to tell the NIC it won't receive fragments on those ports and
thus do RSS. The same way the NIC could be notified if reordering on
this port is possible. This could be done for example via a lightweight
setsockopt.
The situation with encapsulation is even more complicated:
We are basically only interested in the UDP/vxlan/Ethernet/IP/UDP
constellation. If we do the fragmentation inside the vxlan tunnel and
carry over the skb hash to all resulting UDP/vxlan packets source ports,
we are fine and reordering on the receiver NIC won't happen in this
case. If the fragmentation happens on the outer UDP header, this will
result in reordering of the inner L2 flow. Unfortunately this depends on
how the vxlan tunnel was set up, how other devices do that and (I
believe so) on the kernel version.
Given the fact that we tell the NICs the receiving port of vxlan tunnels
anyway (and I only can assume) they are doing RSS on that, we probably
have reordering anyway on those tunnels. So maybe you are right and it
doesn't matter in the end for encapsulated packets, by accident.
> But even before I'd make that recommendation, I'd really like
> understand what the problem actually is. The only thing I can garner
> from this discussion and the Intel patch is that when fragments are
> received OOO that is perceived as a problem. But the by the protocol
> specification clearly says this is not a problem. So the questions
> are: who is negatively affected by this? Is this a problem because
> some artificial test that checks for everything to be in order is now
> failing? Is this affecting real users? Is this an issue in the stack
> or really with some implementation outside of the stack? If it is an
> implementation outside of the stack, then are we just bandaid'ing over
> someone else's incorrect implementation by patching the kernel (like
> would have be the case if we change the kernel to interoperate with
> Facebook's switch that couldn't handle OOO in twstate).
I agree with that and I would also love to hear more feedback on
this. Unfortunately I don't know.
Bye,
Hannes
^ permalink raw reply
* Re: 915f3e3f76 ("mac80211_hwsim: Replace bogus hrtimer clockid"): BUG: kernel reboot-without-warning in test stage
From: Thomas Gleixner @ 2017-09-05 19:30 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: kernel test robot, LKP, linux-kernel, netdev, linux-wireless, wfg
In-Reply-To: <20170905164103.tkrhxzca4b5ulps2@breakpoint.cc>
On Tue, 5 Sep 2017, Sebastian Andrzej Siewior wrote:
> On 2017-09-05 09:12:40 [+0200], Thomas Gleixner wrote:
> > Sorry, no. That bisect is completely bogus. The commit in question merily
> > replaces the unsupported clockid with a valid one.
>
> The bisect is correct. It just has problems to express itself properly. So
> the table says:
>
> | WARNING:at_kernel/time/hrtimer.c:#hrtimer_init | 7 | | | |
> | BUG:kernel_reboot-without-warning_in_test_stage | 0 | 12 | 6 | 6 |
>
> which means _before_ your commit it counted a warning in hrtimer_init()
> (an unsupported clock id was used). With your commit, the warning was
> gone and I *think* the userland then printed
> "BUG:kernel_reboot-without-warning_in_test_stage" because it had no
> warning.
> It seems that the bot learned to live with that warning which was around
> for more than three years. Now that you removed it, it seems to be a
> mistake to do so because nobody complained about it so far.
Thanks for the translation. I'll never learn to decode these reports.
tglx
^ permalink raw reply
* Re: [PATCH 6/10] ixgbe: Use ARRAY_SIZE macro
From: Thomas Meyer @ 2017-09-05 19:45 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20170905.115044.551456011251065741.davem@davemloft.net>
On Tue, Sep 05, 2017 at 11:50:44AM -0700, David Miller wrote:
> From: Thomas Meyer <thomas@m3y3r.de>
> Date: Sun, 03 Sep 2017 14:19:31 +0200
>
> > Use ARRAY_SIZE macro, rather than explicitly coding some variant of it
> > yourself.
> > Found with: find -type f -name "*.c" -o -name "*.h" | xargs perl -p -i -e
> > 's/\bsizeof\s*\(\s*(\w+)\s*\)\s*\ /\s*sizeof\s*\(\s*\1\s*\[\s*0\s*\]\s*\)
> > /ARRAY_SIZE(\1)/g' and manual check/verification.
> >
> > Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
>
> This should be submitted to the Intel ethernet driver maintainers.
Hi,
my script checks the output of get_maintainer scripts and only sends to "open
list" entries.
The intel-wired-lan@lists.osuosl.org is moderated, so that's why the patch
wasn't send there.
Strangely the lists for nouveau@lists.freedesktop.org and
intel-gvt-dev@lists.freedesktop.org appears as open lists in the MAINTAINERS
file but seems to be also moderated lists... At least I got some reply that my
message awaits approval. Maybe an update to the MAINTAINERS file is missing
here?
I may drop above check in my script and send to all mailing lists that
get_maintainer.pl will return.
>
> Thank you.
^ permalink raw reply
* [PATCH][next v2] rocker: fix kcalloc parameter order
From: Zahari Doychev @ 2017-09-05 19:49 UTC (permalink / raw)
To: jiri, netdev; +Cc: Zahari Doychev
The function calls to kcalloc use wrong parameter order and incorrect flags
values. GFP_KERNEL is used instead of flags now and the order is corrected.
The change was done using the following coccinelle script:
@@
expression E1,E2;
type T;
@@
-kcalloc(E1, E2, sizeof(T))
+kcalloc(E2, sizeof(T), GFP_KERNEL)
Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
drivers/net/ethernet/rocker/rocker_ofdpa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
index da4e26b53a52..0653b70723a3 100644
--- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
+++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
@@ -1177,7 +1177,7 @@ static int ofdpa_group_l2_fan_out(struct ofdpa_port *ofdpa_port,
entry->group_id = group_id;
entry->group_count = group_count;
- entry->group_ids = kcalloc(flags, group_count, sizeof(u32));
+ entry->group_ids = kcalloc(group_count, sizeof(u32), GFP_KERNEL);
if (!entry->group_ids) {
kfree(entry);
return -ENOMEM;
@@ -1456,7 +1456,7 @@ static int ofdpa_port_vlan_flood_group(struct ofdpa_port *ofdpa_port,
int err = 0;
int i;
- group_ids = kcalloc(flags, port_count, sizeof(u32));
+ group_ids = kcalloc(port_count, sizeof(u32), GFP_KERNEL);
if (!group_ids)
return -ENOMEM;
--
2.13.0
^ permalink raw reply related
* Re: [PATCH 6/10] ixgbe: Use ARRAY_SIZE macro
From: Joe Perches @ 2017-09-05 20:01 UTC (permalink / raw)
To: Thomas Meyer, David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20170905194419.pdwrkqdad6tbq7hs@olymp>
On Tue, 2017-09-05 at 21:45 +0200, Thomas Meyer wrote:
> On Tue, Sep 05, 2017 at 11:50:44AM -0700, David Miller wrote:
> > From: Thomas Meyer <thomas@m3y3r.de>
> > Date: Sun, 03 Sep 2017 14:19:31 +0200
> >
> > > Use ARRAY_SIZE macro, rather than explicitly coding some variant of it
> > > yourself.
> > > Found with: find -type f -name "*.c" -o -name "*.h" | xargs perl -p -i -e
> > > 's/\bsizeof\s*\(\s*(\w+)\s*\)\s*\ /\s*sizeof\s*\(\s*\1\s*\[\s*0\s*\]\s*\)
> > > /ARRAY_SIZE(\1)/g' and manual check/verification.
> > >
> > > Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
> >
> > This should be submitted to the Intel ethernet driver maintainers.
>
> Hi,
>
> my script checks the output of get_maintainer scripts and only sends to "open
> list" entries.
>
> The intel-wired-lan@lists.osuosl.org is moderated, so that's why the patch
> wasn't send there.
>
> Strangely the lists for nouveau@lists.freedesktop.org and
> intel-gvt-dev@lists.freedesktop.org appears as open lists in the MAINTAINERS
> file but seems to be also moderated lists... At least I got some reply that my
> message awaits approval. Maybe an update to the MAINTAINERS file is missing
> here?
>
> I may drop above check in my script and send to all mailing lists that
> get_maintainer.pl will return.
There's a difference between moderated and subscriber-only
entries in MAINTAINERS.
get_maintainers will by default list moderated lists and
not show subscriber-only lists unless using the -s switch.
^ permalink raw reply
* Re: [PATCH 1/2] b43: fix unitialized reads of ret by initializing the array to zero
From: Michael Büsch @ 2017-09-05 20:18 UTC (permalink / raw)
To: Colin King
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kalle Valo
In-Reply-To: <20170905181550.23839-1-colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1512 bytes --]
On Tue, 5 Sep 2017 19:15:50 +0100
Colin King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values. Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
>
> Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
>
> Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
> drivers/net/wireless/broadcom/b43/phy_g.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/b43/phy_g.c b/drivers/net/wireless/broadcom/b43/phy_g.c
> index 822dcaa8ace6..f59c02166462 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_g.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_g.c
> @@ -2297,7 +2297,7 @@ static u8 b43_gphy_aci_detect(struct b43_wldev *dev, u8 channel)
> static u8 b43_gphy_aci_scan(struct b43_wldev *dev)
> {
> struct b43_phy *phy = &dev->phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
> unsigned int channel = phy->channel;
> unsigned int i, j, start, end;
>
This fix seems to be correct.
Thanks for finding and fixing the issue.
Reviewed-by: Michael Buesch <m@bues.ch>
--
Michael
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 179 bytes --]
_______________________________________________
b43-dev mailing list
b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/b43-dev
^ permalink raw reply
* Re: [PATCH 2/2] b43legacy: fix unitialized reads of ret by initializing the array to zero
From: Michael Büsch @ 2017-09-05 20:18 UTC (permalink / raw)
To: Colin King
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kalle Valo,
Larry Finger
In-Reply-To: <20170905181658.23893-1-colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1561 bytes --]
On Tue, 5 Sep 2017 19:16:58 +0100
Colin King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values. Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
>
> Detected by CoverityScan CID#139653 ("Uninitialized scalar variable")
>
> Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
> drivers/net/wireless/broadcom/b43legacy/radio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/b43legacy/radio.c b/drivers/net/wireless/broadcom/b43legacy/radio.c
> index 9501420340a9..eab1c9387846 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/radio.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/radio.c
> @@ -280,7 +280,7 @@ u8 b43legacy_radio_aci_detect(struct b43legacy_wldev *dev, u8 channel)
> u8 b43legacy_radio_aci_scan(struct b43legacy_wldev *dev)
> {
> struct b43legacy_phy *phy = &dev->phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
> unsigned int channel = phy->channel;
> unsigned int i;
> unsigned int j;
This fix seems to be correct.
Thanks for finding and fixing the issue.
Reviewed-by: Michael Buesch <m@bues.ch>
--
Michael
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 179 bytes --]
_______________________________________________
b43-dev mailing list
b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/b43-dev
^ permalink raw reply
* [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
From: Kardonik Michael @ 2017-09-05 20:27 UTC (permalink / raw)
To: leoyang.li, michael.kardonik, Jeff Kirsher, Jesse Brandeburg,
Shannon Nelson, Carolyn Wyborny, Don Skidmore, Matthew Vick,
John Ronciak, Mitch Williams, intel-wired-lan, netdev,
linux-kernel
Cc: Matthew Tan
Calls to udelay are not preemtable by userspace so userspace
applications experience a large (~200us) latency when running on core0.
Instead usleep_range can be used to be more friendly to userspace
since it is preemtable. This is due to udelay using busy-wait loops
while usleep_rang uses hrtimers instead. It is recommended to use
udelay when the delay is <10us since at that precision overhead of
usleep_range hrtimer setup causes issues. However, the replaced calls
are for 50us and 100us so this should not be not an issue.
There is no open bug that this patch is fixing, but we see a good
boost in zero loss performance of specific user space application
(dpdk l3fwd) when this patch is applied: we get from 32% of 10Gb line
to 49%.
Signed-off-by: Matthew Tan <matthew.tan_1@nxp.com>
Signed-off-by: Michael Kardonik <michael.kardonik@nxp.com>
---
drivers/net/ethernet/intel/e1000e/phy.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
index de13aea..e318fdc 100644
--- a/drivers/net/ethernet/intel/e1000e/phy.c
+++ b/drivers/net/ethernet/intel/e1000e/phy.c
@@ -158,7 +158,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
* the lower time out
*/
for (i = 0; i < (E1000_GEN_POLL_TIMEOUT * 3); i++) {
- udelay(50);
+ usleep_range(50, 60);
mdic = er32(MDIC);
if (mdic & E1000_MDIC_READY)
break;
@@ -183,7 +183,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
* reading duplicate data in the next MDIC transaction.
*/
if (hw->mac.type == e1000_pch2lan)
- udelay(100);
+ usleep_range(100, 110);
return 0;
}
@@ -222,7 +222,7 @@ s32 e1000e_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
* the lower time out
*/
for (i = 0; i < (E1000_GEN_POLL_TIMEOUT * 3); i++) {
- udelay(50);
+ usleep_range(50, 60);
mdic = er32(MDIC);
if (mdic & E1000_MDIC_READY)
break;
@@ -246,7 +246,7 @@ s32 e1000e_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
* reading duplicate data in the next MDIC transaction.
*/
if (hw->mac.type == e1000_pch2lan)
- udelay(100);
+ usleep_range(100, 110);
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 2/4] ip/options: explicitly provide net ns to __ip_options_echo()
From: Paolo Abeni @ 2017-09-05 21:03 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller; +Cc: netdev, Eric Dumazet, Hannes Frederic Sowa
In-Reply-To: <1504631927.15310.47.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, 2017-09-05 at 10:18 -0700, Eric Dumazet wrote:
> On Thu, 2017-08-03 at 18:07 +0200, Paolo Abeni wrote:
> > __ip_options_echo() uses the current network namespace, and
> > currently retrives it via skb->dst->dev.
> >
> > This commit adds an explicit 'net' argument to __ip_options_echo()
> > and update all the call sites to provide it, usually via a simpler
> > sock_net().
> >
> > After this change, __ip_options_echo() no more needs to access
> > skb->dst and we can drop a couple of hack to preserve such
> > info in the rx path.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
>
> David, Paolo
>
> This commit (91ed1e666a4ea2e260452a7d7d311ac5ae852cba "ip/options:
> explicitly provide net ns to __ip_options_echo()")
>
> needs to be backported to linux-4.13 stable version to avoid these kind
> of crashes [1]
>
> This is because of MSG_PEEK operation, hitting skb_consume_udp() while
> skb is still in receive queue.
>
> Next read() finding again the skb then can see a NULL skb->dst
>
> Thanks !
>
> [1]
> general protection fault: 0000 [#1] SMP KASAN
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Modules linked in:
> CPU: 0 PID: 3017 Comm: syzkaller446772 Not tainted 4.13.0+ #68
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> task: ffff8801cd0a4380 task.stack: ffff8801cc498000
> RIP: 0010:__ip_options_echo+0xea8/0x1430 net/ipv4/ip_options.c:143
> RSP: 0018:ffff8801cc49f628 EFLAGS: 00010246
> RAX: dffffc0000000000 RBX: ffff8801cc49f928 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000004
> RBP: ffff8801cc49f6b8 R08: ffff8801cc49f936 R09: ffffed0039893f28
> R10: 0000000000000003 R11: ffffed0039893f27 R12: ffff8801cc49f918
> R13: ffff8801ccbcf36c R14: 000000000000000f R15: 0000000000000018
> FS: 0000000000979880(0000) GS:ffff8801db200000(0000)
> knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000200c0ff0 CR3: 00000001cc4ed000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ip_options_echo include/net/ip.h:574 [inline]
> ip_cmsg_recv_retopts net/ipv4/ip_sockglue.c:91 [inline]
> ip_cmsg_recv_offset+0xa17/0x1280 net/ipv4/ip_sockglue.c:207
> udp_recvmsg+0xe0b/0x1260 net/ipv4/udp.c:1641
> inet_recvmsg+0x14c/0x5f0 net/ipv4/af_inet.c:793
> sock_recvmsg_nosec net/socket.c:792 [inline]
> sock_recvmsg+0xc9/0x110 net/socket.c:799
> SYSC_recvfrom+0x2dc/0x570 net/socket.c:1788
> SyS_recvfrom+0x40/0x50 net/socket.c:1760
> entry_SYSCALL_64_fastpath+0x1f/0xbe
> RIP: 0033:0x444c89
> RSP: 002b:00007ffd80c788e8 EFLAGS: 00000286 ORIG_RAX: 000000000000002d
> RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 0000000000444c89
> RDX: 0000000000000000 RSI: 0000000020bc0000 RDI: 0000000000000004
> RBP: 0000000000000082 R08: 00000000200c0ff0 R09: 0000000000000010
> R10: 0000000000000000 R11: 0000000000000286 R12: 0000000000402390
> R13: 0000000000402420 R14: 0000000000000000 R15: 0000000000000000
> Code: f6 c1 01 0f 85 a5 01 00 00 48 89 4d b8 e8 31 e9 6b fd 48 8b 4d b8
> 48 b8 00 00 00 00 00 fc ff df 48 83 e1 fe 48 89 ca 48 c1 ea 03 <80> 3c
> 02 00 0f 85 41 02 00 00 48 8b 09 48 b8 00 00 00 00 00 fc
> RIP: __ip_options_echo+0xea8/0x1430 net/ipv4/ip_options.c:143 RSP:
> ffff8801cc49f628
> ---[ end trace b30d95b284222843 ]---
> Kernel panic - not syncing: Fatal exception
Thank you Eric for the report!
Darn me, I seriously messed-up with the stateless consume.
I think we can have similar issues pith ipsec/secpath and MSG_PEEK,
even if with less catastropthic outcome.
What about the following, which should cover both cases? (only compile
tested, I'll test it tomorrow morning my time)
---
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d67a8182e5eb..63df75ae70ee 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -885,7 +885,7 @@ void kfree_skb(struct sk_buff *skb);
void kfree_skb_list(struct sk_buff *segs);
void skb_tx_error(struct sk_buff *skb);
void consume_skb(struct sk_buff *skb);
-void consume_stateless_skb(struct sk_buff *skb);
+void __consume_stateless_skb(struct sk_buff *skb);
void __kfree_skb(struct sk_buff *skb);
extern struct kmem_cache *skbuff_head_cache;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e07556606284..f2411a8744d7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -753,14 +753,11 @@ EXPORT_SYMBOL(consume_skb);
* consume_stateless_skb - free an skbuff, assuming it is stateless
* @skb: buffer to free
*
- * Works like consume_skb(), but this variant assumes that all the head
- * states have been already dropped.
+ * Alike consume_skb(), but this variant assumes that all the head
+ * states have been already dropped and usage count is one
*/
-void consume_stateless_skb(struct sk_buff *skb)
+void __consume_stateless_skb(struct sk_buff *skb)
{
- if (!skb_unref(skb))
- return;
-
trace_consume_skb(skb);
if (likely(skb->head))
skb_release_data(skb);
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 62344804baae..979e4d8526ba 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1386,12 +1386,15 @@ void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len)
unlock_sock_fast(sk, slow);
}
+ if (!skb_unref(skb))
+ return;
+
/* In the more common cases we cleared the head states previously,
* see __udp_queue_rcv_skb().
*/
if (unlikely(udp_skb_has_head_state(skb)))
skb_release_head_state(skb);
- consume_stateless_skb(skb);
+ __consume_stateless_skb(skb);
}
EXPORT_SYMBOL_GPL(skb_consume_udp);
^ permalink raw reply related
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Tom Herbert @ 2017-09-05 21:13 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <87fuc1q7en.fsf@stressinduktion.org>
> The situation with encapsulation is even more complicated:
>
> We are basically only interested in the UDP/vxlan/Ethernet/IP/UDP
> constellation. If we do the fragmentation inside the vxlan tunnel and
> carry over the skb hash to all resulting UDP/vxlan packets source ports,
> we are fine and reordering on the receiver NIC won't happen in this
> case. If the fragmentation happens on the outer UDP header, this will
> result in reordering of the inner L2 flow. Unfortunately this depends on
> how the vxlan tunnel was set up, how other devices do that and (I
> believe so) on the kernel version.
>
This really isn't that complicated. The assumption that an IP network
always delivers packets in order is simply wrong. The inventors of
VXLAN must have know full well that when you use IP, packets can and
eventually will be delivered out of order. This isn't just because of
fragmentation, there are many other reasons that packets can be
delivered OOO. This also must have been known when IP/GRE and any
other protocol that carries L2 over IP was invented. If OOO is an
issue for these protocols then they need to be fixed-- this is not a
concern with IP protocol nor the stack.
Tom
^ 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