* Re: [PATCH net-next] net: stmmac: pci: Add HAPS support using GMAC5
From: David Miller @ 2019-09-11 22:50 UTC (permalink / raw)
To: Jose.Abreu
Cc: netdev, Joao.Pinto, peppe.cavallaro, alexandre.torgue,
mcoquelin.stm32, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <c37a55225e1ef66233b47c02b1441b91abeb3b76.1568047994.git.joabreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Mon, 9 Sep 2019 18:54:26 +0200
> Add the support for Synopsys HAPS board that uses GMAC5.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: Don't use dst gateway directly in ip6_confirm_neigh()
From: David Miller @ 2019-09-11 22:52 UTC (permalink / raw)
To: sbrivio; +Cc: gnault, ja, nicolas.dichtel, dsahern, netdev
In-Reply-To: <938b711c35ce3fa2b6f057cc23919e897a1e5c2b.1568061608.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Mon, 9 Sep 2019 22:44:06 +0200
> This is the equivalent of commit 2c6b55f45d53 ("ipv6: fix neighbour
> resolution with raw socket") for ip6_confirm_neigh(): we can send a
> packet with MSG_CONFIRM on a raw socket for a connected route, so the
> gateway would be :: here, and we should pick the next hop using
> rt6_nexthop() instead.
>
> This was found by code review and, to the best of my knowledge, doesn't
> actually fix a practical issue: the destination address from the packet
> is not considered while confirming a neighbour, as ip6_confirm_neigh()
> calls choose_neigh_daddr() without passing the packet, so there are no
> similar issues as the one fixed by said commit.
>
> A possible source of issues with the existing implementation might come
> from the fact that, if we have a cached dst, we won't consider it,
> while rt6_nexthop() takes care of that. I might just not be creative
> enough to find a practical problem here: the only way to affect this
> with cached routes is to have one coming from an ICMPv6 redirect, but
> if the next hop is a directly connected host, there should be no
> topology for which a redirect applies here, and tests with redirected
> routes show no differences for MSG_CONFIRM (and MSG_PROBE) packets on
> raw sockets destined to a directly connected host.
>
> However, directly using the dst gateway here is not consistent anymore
> with neighbour resolution, and, in general, as we want the next hop,
> using rt6_nexthop() looks like the only sane way to fetch it.
>
> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/7] net/dsa: configure autoneg for CPU port
From: Andrew Lunn @ 2019-09-11 22:52 UTC (permalink / raw)
To: Robert Beckett; +Cc: Florian Fainelli, netdev, Vivien Didelot, David S. Miller
In-Reply-To: <ad302835a98ca5abc7ac88b3caad64867e33ee70.camel@collabora.com>
> It is not just for broadcast storm protection. The original issue that
> made me look in to all of this turned out to be rx descritor ring
> buffer exhaustion due to the CPU not being able to keep up with packet
> reception.
Pause frames does not really solve this problem. The switch will at
some point fill its buffers, and start throwing packets away. Or it
needs to send pause packets it its peers. And then your whole switch
throughput goes down. Packets will always get thrown away, so you need
QoS in your network to give the network hints about which frames is
should throw away first.
..
> Fundamentally, with a phy to phy CPU connection, the CPU MAC may well
> wish to enable pause frames for various reasons, so we should strive to
> handle that I think.
It actually has nothing to do with PHY to PHY connections. You can use
pause frames with direct MAC to MAC connections. PHY auto-negotiation
is one way to indicate both ends support it, but there are also other
ways. e.g.
ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]
on the SoC you could do
ethtool --pause eth0 autoneg off rx on tx on
to force the SoC to send and process pause frames. Ideally i would
prefer a solution like this, since it is not a change of behaviour for
everybody else.
Andrew
^ permalink raw reply
* Re: [PATCH net] tcp: fix tcp_ecn_withdraw_cwr() to clear TCP_ECN_QUEUE_CWR
From: David Miller @ 2019-09-11 22:54 UTC (permalink / raw)
To: ncardwell; +Cc: netdev, ycheng, soheil, edumazet
In-Reply-To: <20190909205602.248472-1-ncardwell@google.com>
From: Neal Cardwell <ncardwell@google.com>
Date: Mon, 9 Sep 2019 16:56:02 -0400
> Fix tcp_ecn_withdraw_cwr() to clear the correct bit:
> TCP_ECN_QUEUE_CWR.
>
> Rationale: basically, TCP_ECN_DEMAND_CWR is a bit that is purely about
> the behavior of data receivers, and deciding whether to reflect
> incoming IP ECN CE marks as outgoing TCP th->ece marks. The
> TCP_ECN_QUEUE_CWR bit is purely about the behavior of data senders,
> and deciding whether to send CWR. The tcp_ecn_withdraw_cwr() function
> is only called from tcp_undo_cwnd_reduction() by data senders during
> an undo, so it should zero the sender-side state,
> TCP_ECN_QUEUE_CWR. It does not make sense to stop the reflection of
> incoming CE bits on incoming data packets just because outgoing
> packets were spuriously retransmitted.
>
> The bug has been reproduced with packetdrill to manifest in a scenario
> with RFC3168 ECN, with an incoming data packet with CE bit set and
> carrying a TCP timestamp value that causes cwnd undo. Before this fix,
> the IP CE bit was ignored and not reflected in the TCP ECE header bit,
> and sender sent a TCP CWR ('W') bit on the next outgoing data packet,
> even though the cwnd reduction had been undone. After this fix, the
> sender properly reflects the CE bit and does not set the W bit.
>
> Note: the bug actually predates 2005 git history; this Fixes footer is
> chosen to be the oldest SHA1 I have tested (from Sep 2007) for which
> the patch applies cleanly (since before this commit the code was in a
> .h file).
>
> Fixes: bdf1ee5d3bd3 ("[TCP]: Move code from tcp_ecn.h to tcp*.c and tcp.h & remove it")
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
> Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
Applied and queued up for -stable, thanks Neal.
^ permalink raw reply
* Re: [PATCH 0/7] net: dsa: mv88e6xxx: features to handle network storms
From: Andrew Lunn @ 2019-09-11 22:58 UTC (permalink / raw)
To: Robert Beckett
Cc: Ido Schimmel, Florian Fainelli, netdev@vger.kernel.org,
Vivien Didelot, David S. Miller, Jiri Pirko
In-Reply-To: <3f50ee51ec04a2d683a5338a68607824a3f45711.camel@collabora.com>
> We have a setup as follows:
>
> Marvell 88E6240 switch chip, accepting traffic from 4 ports. Port 1
> (P1) is critical priority, no dropped packets allowed, all others can
> be best effort.
>
> CPU port of swtich chip is connected via phy to phy of intel i210 (igb
> driver).
>
> i210 is connected via pcie switch to imx6.
>
> When too many small packets attempt to be delivered to CPU port (e.g.
> during broadcast flood) we saw dropped packets.
>
> The packets were being received by i210 in to rx descriptor buffer
> fine, but the CPU could not keep up with the load. We saw
> rx_fifo_errors increasing rapidly and ksoftirqd at ~100% CPU.
>
>
> With this in mind, I am wondering whether any amount of tc traffic
> shaping would help?
Hi Robert
The model in linux is that you start with a software TC filter, and
then offload it to the hardware. So the user configures TC just as
normal, and then that is used to program the hardware to do the same
thing as what would happen in software. This is exactly the same as we
do with bridging. You create a software bridge and add interfaces to
the bridge. This then gets offloaded to the hardware and it does the
bridging for you.
So think about how your can model the Marvell switch capabilities
using TC, and implement offload support for it.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] nfp: read chip model from the PluDevice register
From: David Miller @ 2019-09-11 23:01 UTC (permalink / raw)
To: simon.horman; +Cc: jakub.kicinski, netdev, oss-drivers, dirk.vandermerwe
In-Reply-To: <20190911152118.30698-1-simon.horman@netronome.com>
From: Simon Horman <simon.horman@netronome.com>
Date: Wed, 11 Sep 2019 16:21:18 +0100
> From: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
>
> The PluDevice register provides the authoritative chip model/revision.
>
> Since the model number is purely used for reporting purposes, follow
> the hardware team convention of subtracting 0x10 from the PluDevice
> register to obtain the chip model/revision number.
>
> Suggested-by: Francois H. Theron <francois.theron@netronome.com>
> Signed-off-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
Applied.
^ permalink raw reply
* Re: [PATCH 0/7] net: dsa: mv88e6xxx: features to handle network storms
From: Andrew Lunn @ 2019-09-11 23:01 UTC (permalink / raw)
To: Robert Beckett; +Cc: Vivien Didelot, netdev, Florian Fainelli, David S. Miller
In-Reply-To: <3f265c5afcb2eea48410ec607d65e8f4e6a20373.camel@collabora.com>
> > Feature series targeting netdev must be prefixed "PATCH net-next". As
>
> Thanks for the info. Out of curiosity, where should I have gleaned this
> info from? This is my first contribution to netdev, so I wasnt familiar
> with the etiquette.
It is also a good idea to 'lurk' in a mailing list for a while,
reading emails flying around, getting to know how things work. This
subject of "PATCH net-next" comes up maybe once a week. The idea off
offloads gets discussed once every couple of weeks etc.
Andrew
^ permalink raw reply
* Re: [net 0/2][pull request] Intel Wired LAN Driver Updates 2019-09-11
From: David Miller @ 2019-09-11 23:08 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann
In-Reply-To: <20190911164955.10644-1-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 11 Sep 2019 09:49:53 -0700
> This series contains fixes to ixgbe.
>
> Alex fixes up the adaptive ITR scheme for ixgbe which could result in a
> value that was either 0 or something less than 10 which was causing
> issues with hardware features, like RSC, that do not function well with
> ITR values that low.
>
> Ilya Maximets fixes the ixgbe driver to limit the number of transmit
> descriptors to clean by the number of transmit descriptors used in the
> transmit ring, so that the driver does not try to "double" clean the
> same descriptors.
Pulled, thanks Jeff.
^ permalink raw reply
* [PATCH][PATCH net-next] hv_sock: Add the support of hibernation
From: Dexuan Cui @ 2019-09-11 23:37 UTC (permalink / raw)
To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, davem@davemloft.net,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
Cc: Dexuan Cui
Add the necessary dummy callbacks for hibernation.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
This patch is basically a pure Hyper-V specific change and it has a
build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement
suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
Hyper-V tree's hyperv-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
I request this patch should go through Sasha's tree rather than the
net-next tree.
net/vmw_vsock/hyperv_transport.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index f2084e3..e91a884 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -930,6 +930,24 @@ static int hvs_remove(struct hv_device *hdev)
return 0;
}
+/* hv_sock connections can not persist across hibernation, and all the hv_sock
+ * channels are forceed to be rescinded before hibernation: see
+ * vmbus_bus_suspend(). Here the dummy hvs_suspend() and hvs_resume()
+ * are only needed because hibernation requires that every device's driver
+ * should have a .suspend and .resume callback: see vmbus_suspend().
+ */
+static int hvs_suspend(struct hv_device *hv_dev)
+{
+ /* Dummy */
+ return 0;
+}
+
+static int hvs_resume(struct hv_device *dev)
+{
+ /* Dummy */
+ return 0;
+}
+
/* This isn't really used. See vmbus_match() and vmbus_probe() */
static const struct hv_vmbus_device_id id_table[] = {
{},
@@ -941,6 +959,8 @@ static int hvs_remove(struct hv_device *hdev)
.id_table = id_table,
.probe = hvs_probe,
.remove = hvs_remove,
+ .suspend = hvs_suspend,
+ .resume = hvs_resume,
};
static int __init hvs_init(void)
--
1.8.3.1
^ permalink raw reply related
* [PATCH][PATCH net-next] hv_netvsc: Add the support of hibernation
From: Dexuan Cui @ 2019-09-11 23:37 UTC (permalink / raw)
To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, davem@davemloft.net,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley
Cc: Dexuan Cui
The existing netvsc_detach() and netvsc_attach() APIs make it easy to
implement the suspend/resume callbacks.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
This patch is basically a pure Hyper-V specific change and it has a
build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement
suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
Hyper-V tree's hyperv-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
I request this patch should go through Sasha's tree rather than the
net-next tree.
drivers/net/hyperv/hyperv_net.h | 3 +++
drivers/net/hyperv/netvsc_drv.c | 59 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index ecc9af0..b8763ee 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -952,6 +952,9 @@ struct net_device_context {
u32 vf_alloc;
/* Serial number of the VF to team with */
u32 vf_serial;
+
+ /* Used to temporarily save the config info across hibernation */
+ struct netvsc_device_info *saved_netvsc_dev_info;
};
/* Per channel data */
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index afdcc56..f920959 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2392,6 +2392,63 @@ static int netvsc_remove(struct hv_device *dev)
return 0;
}
+static int netvsc_suspend(struct hv_device *dev)
+{
+ struct net_device_context *ndev_ctx;
+ struct net_device *vf_netdev, *net;
+ struct netvsc_device *nvdev;
+ int ret;
+
+ net = hv_get_drvdata(dev);
+
+ ndev_ctx = netdev_priv(net);
+ cancel_delayed_work_sync(&ndev_ctx->dwork);
+
+ rtnl_lock();
+
+ nvdev = rtnl_dereference(ndev_ctx->nvdev);
+ if (nvdev == NULL) {
+ ret = -ENODEV;
+ goto out;
+ }
+
+ cancel_work_sync(&nvdev->subchan_work);
+
+ vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
+ if (vf_netdev)
+ netvsc_unregister_vf(vf_netdev);
+
+ /* Save the current config info */
+ ndev_ctx->saved_netvsc_dev_info = netvsc_devinfo_get(nvdev);
+
+ ret = netvsc_detach(net, nvdev);
+out:
+ rtnl_unlock();
+
+ return ret;
+}
+
+static int netvsc_resume(struct hv_device *dev)
+{
+ struct net_device *net = hv_get_drvdata(dev);
+ struct net_device_context *net_device_ctx;
+ struct netvsc_device_info *device_info;
+ int ret;
+
+ rtnl_lock();
+
+ net_device_ctx = netdev_priv(net);
+ device_info = net_device_ctx->saved_netvsc_dev_info;
+
+ ret = netvsc_attach(net, device_info);
+
+ rtnl_unlock();
+
+ kfree(device_info);
+ net_device_ctx->saved_netvsc_dev_info = NULL;
+
+ return ret;
+}
static const struct hv_vmbus_device_id id_table[] = {
/* Network guid */
{ HV_NIC_GUID, },
@@ -2406,6 +2463,8 @@ static int netvsc_remove(struct hv_device *dev)
.id_table = id_table,
.probe = netvsc_probe,
.remove = netvsc_remove,
+ .suspend = netvsc_suspend,
+ .resume = netvsc_resume,
.driver = {
.probe_type = PROBE_FORCE_SYNCHRONOUS,
},
--
1.8.3.1
^ permalink raw reply related
* Klientskie Bazy http://prodawez.tilda.ws/page7270311.html
From: netdev @ 2019-09-12 0:25 UTC (permalink / raw)
To: netdev
Klientskie Bazy http://prodawez.tilda.ws/page7270311.html
^ permalink raw reply
* Re: [PATCH v3 2/2] tcp: Add rcv_wnd to TCP_INFO
From: Neal Cardwell @ 2019-09-12 0:49 UTC (permalink / raw)
To: Thomas Higdon
Cc: netdev@vger.kernel.org, Jonathan Lemon, Dave Jones, Eric Dumazet,
Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190911223148.89808-2-tph@fb.com>
On Wed, Sep 11, 2019 at 6:32 PM Thomas Higdon <tph@fb.com> wrote:
>
> Neal Cardwell mentioned that rcv_wnd would be useful for helping
> diagnose whether a flow is receive-window-limited at a given instant.
>
> This serves the purpose of adding an additional __u32 to avoid the
> would-be hole caused by the addition of the tcpi_rcvi_ooopack field.
>
> Signed-off-by: Thomas Higdon <tph@fb.com>
> ---
Thanks, Thomas.
I know that when I mentioned this before I mentioned the idea of both
tp->snd_wnd (send-side receive window) and tp->rcv_wnd (receive-side
receive window) in tcp_info, and did not express a preference between
the two. Now that we are faced with a decision between the two,
personally I think it would be a little more useful to start with
tp->snd_wnd. :-)
Two main reasons:
(1) Usually when we're diagnosing TCP performance problems, we do so
from the sender, since the sender makes most of the
performance-critical decisions (cwnd, pacing, TSO size, TSQ, etc).
From the sender-side the thing that would be most useful is to see
tp->snd_wnd, the receive window that the receiver has advertised to
the sender.
(2) From the receiver side, "ss" can already show a fair amount of
info about receive-side buffer/window limits, like:
info->tcpi_rcv_ssthresh, info->tcpi_rcv_space,
skmeminfo[SK_MEMINFO_RMEM_ALLOC], skmeminfo[SK_MEMINFO_RCVBUF]. Often
the rwin can be approximated by combining those.
Hopefully Eric, Yuchung, and Soheil can weigh in on the question of
snd_wnd vs rcv_wnd. Or we can perhaps think of another field, and add
the tcpi_rcvi_ooopack, snd_wnd, rcv_wnd, and that final field, all
together.
thanks,
neal
^ permalink raw reply
* Re: [PATCH V2 net-next 4/7] net: hns3: fix port setting handle for fibre port
From: tanhuazhong @ 2019-09-12 0:56 UTC (permalink / raw)
To: Sergei Shtylyov, davem
Cc: netdev, linux-kernel, salil.mehta, yisen.zhuang, linuxarm,
jakub.kicinski
In-Reply-To: <7f914173-a2fc-08d8-e2b1-48fa3da4e29c@cogentembedded.com>
On 2019/9/11 18:16, Sergei Shtylyov wrote:
> Hello!
>
> On 11.09.2019 5:40, Huazhong Tan wrote:
>
>> From: Guangbin Huang <huangguangbin2@huawei.com>
>>
>> For hardware doesn't support use specified speed and duplex
>
> Can't pasre that. "For hardware that does not support using", perhaps?
Yes, thanks. Will check the grammar more carefully next time.
>
>> to negotiate, it's unnecessary to check and modify the port
>> speed and duplex for fibre port when autoneg is on.
>>
>> Fixes: 22f48e24a23d ("net: hns3: add autoneg and change speed support
>> for fibre port")
>> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
>> ---
>> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> index f5a681d..680c350 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> @@ -726,6 +726,12 @@ static int hns3_check_ksettings_param(const
>> struct net_device *netdev,
>> u8 duplex;
>> int ret;
>> + /* hw doesn't support use specified speed and duplex to negotiate,
>
> I can't parse that, did you mean "using"?
yes, thanks.
>
>> + * unnecessary to check them when autoneg on.
>> + */
>> + if (cmd->base.autoneg)
>> + return 0;
>> +
>> if (ops->get_ksettings_an_result) {
>> ops->get_ksettings_an_result(handle, &autoneg, &speed,
>> &duplex);
>> if (cmd->base.autoneg == autoneg && cmd->base.speed == speed &&
>> @@ -787,6 +793,15 @@ static int hns3_set_link_ksettings(struct
>> net_device *netdev,
>> return ret;
>> }
>> + /* hw doesn't support use specified speed and duplex to negotiate,
>
> Here too...
>
yes, thanks.
>> + * ignore them when autoneg on.
>> + */
>> + if (cmd->base.autoneg) {
>> + netdev_info(netdev,
>> + "autoneg is on, ignore the speed and duplex\n");
>> + return 0;
>> + }
>> +
>> if (ops->cfg_mac_speed_dup_h)
>> ret = ops->cfg_mac_speed_dup_h(handle, cmd->base.speed,
>> cmd->base.duplex);
>
> MBR, Sergei
>
> .
>
^ permalink raw reply
* Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()
From: Cong Wang @ 2019-09-12 1:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Network Developers, syzbot, Linus Torvalds,
Jamal Hadi Salim, Jiri Pirko
In-Reply-To: <7b5b69a9-7ace-2d21-f187-7a81fb1dae5a@gmail.com>
On Wed, Sep 11, 2019 at 2:36 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> It seems a similar fix would be needed in net/sched/sch_dsmark.c ?
>
Yeah, or just add a NULL check in dsmark_destroy().
Anyway, I will send a separate patch for it.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 net-next 12/15] net: dsa: sja1105: Configure the Time-Aware Scheduler via tc-taprio offload
From: Vladimir Oltean @ 2019-09-12 1:30 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: f.fainelli, vivien.didelot, andrew, davem, vedang.patel,
richardcochran, weifeng.voon, jiri, m-karicheri2, Jose.Abreu,
ilias.apalodimas, jhs, xiyou.wangcong, kurt.kanzenbach, netdev
In-Reply-To: <87woeeipm8.fsf@linux.intel.com>
Hi Vinicius,
On 11/09/2019, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
> Hi,
>
> Vladimir Oltean <olteanv@gmail.com> writes:
>
>> This qdisc offload is the closest thing to what the SJA1105 supports in
>> hardware for time-based egress shaping. The switch core really is built
>> around SAE AS6802/TTEthernet (a TTTech standard) but can be made to
>> operate similarly to IEEE 802.1Qbv with some constraints:
>>
>> - The gate control list is a global list for all ports. There are 8
>> execution threads that iterate through this global list in parallel.
>> I don't know why 8, there are only 4 front-panel ports.
>>
>> - Care must be taken by the user to make sure that two execution threads
>> never get to execute a GCL entry simultaneously. I created a O(n^4)
>> checker for this hardware limitation, prior to accepting a taprio
>> offload configuration as valid.
>>
>> - The spec says that if a GCL entry's interval is shorter than the frame
>> length, you shouldn't send it (and end up in head-of-line blocking).
>> Well, this switch does anyway.
>>
>> - The switch has no concept of ADMIN and OPER configurations. Because
>> it's so simple, the TAS settings are loaded through the static config
>> tables interface, so there isn't even place for any discussion about
>> 'graceful switchover between ADMIN and OPER'. You just reset the
>> switch and upload a new OPER config.
>>
>> - The switch accepts multiple time sources for the gate events. Right
>> now I am using the standalone clock source as opposed to PTP. So the
>> base time parameter doesn't really do much. Support for the PTP clock
>> source will be added in the next patch.
>>
>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
>> Changes since RFC:
>> - Removed the sja1105_tas_config_work workqueue.
>> - Allocating memory with GFP_KERNEL.
>> - Made the ASCII art drawing fit in < 80 characters.
>> - Made most of the time-holding variables s64 instead of u64 (for fear
>> of them not holding the result of signed arithmetics properly).
>>
>> drivers/net/dsa/sja1105/Kconfig | 8 +
>> drivers/net/dsa/sja1105/Makefile | 4 +
>> drivers/net/dsa/sja1105/sja1105.h | 5 +
>> drivers/net/dsa/sja1105/sja1105_main.c | 19 +-
>> drivers/net/dsa/sja1105/sja1105_tas.c | 420 +++++++++++++++++++++++++
>> drivers/net/dsa/sja1105/sja1105_tas.h | 42 +++
>> 6 files changed, 497 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/net/dsa/sja1105/sja1105_tas.c
>> create mode 100644 drivers/net/dsa/sja1105/sja1105_tas.h
>>
>> diff --git a/drivers/net/dsa/sja1105/Kconfig
>> b/drivers/net/dsa/sja1105/Kconfig
>> index 770134a66e48..55424f39cb0d 100644
>> --- a/drivers/net/dsa/sja1105/Kconfig
>> +++ b/drivers/net/dsa/sja1105/Kconfig
>> @@ -23,3 +23,11 @@ config NET_DSA_SJA1105_PTP
>> help
>> This enables support for timestamping and PTP clock manipulations in
>> the SJA1105 DSA driver.
>> +
>> +config NET_DSA_SJA1105_TAS
>> + bool "Support for the Time-Aware Scheduler on NXP SJA1105"
>> + depends on NET_DSA_SJA1105
>> + help
>> + This enables support for the TTEthernet-based egress scheduling
>> + engine in the SJA1105 DSA driver, which is controlled using a
>> + hardware offload of the tc-tqprio qdisc.
>> diff --git a/drivers/net/dsa/sja1105/Makefile
>> b/drivers/net/dsa/sja1105/Makefile
>> index 4483113e6259..66161e874344 100644
>> --- a/drivers/net/dsa/sja1105/Makefile
>> +++ b/drivers/net/dsa/sja1105/Makefile
>> @@ -12,3 +12,7 @@ sja1105-objs := \
>> ifdef CONFIG_NET_DSA_SJA1105_PTP
>> sja1105-objs += sja1105_ptp.o
>> endif
>> +
>> +ifdef CONFIG_NET_DSA_SJA1105_TAS
>> +sja1105-objs += sja1105_tas.o
>> +endif
>> diff --git a/drivers/net/dsa/sja1105/sja1105.h
>> b/drivers/net/dsa/sja1105/sja1105.h
>> index 3ca0b87aa3e4..d95f9ce3b4f9 100644
>> --- a/drivers/net/dsa/sja1105/sja1105.h
>> +++ b/drivers/net/dsa/sja1105/sja1105.h
>> @@ -21,6 +21,7 @@
>> #define SJA1105_AGEING_TIME_MS(ms) ((ms) / 10)
>>
>> #include "sja1105_ptp.h"
>> +#include "sja1105_tas.h"
>>
>> /* Keeps the different addresses between E/T and P/Q/R/S */
>> struct sja1105_regs {
>> @@ -96,6 +97,7 @@ struct sja1105_private {
>> struct mutex mgmt_lock;
>> struct sja1105_tagger_data tagger_data;
>> struct sja1105_ptp_data ptp_data;
>> + struct sja1105_tas_data tas_data;
>> };
>>
>> #include "sja1105_dynamic_config.h"
>> @@ -111,6 +113,9 @@ typedef enum {
>> SPI_WRITE = 1,
>> } sja1105_spi_rw_mode_t;
>>
>> +/* From sja1105_main.c */
>> +int sja1105_static_config_reload(struct sja1105_private *priv);
>> +
>> /* From sja1105_spi.c */
>> int sja1105_spi_send_packed_buf(const struct sja1105_private *priv,
>> sja1105_spi_rw_mode_t rw, u64 reg_addr,
>> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c
>> b/drivers/net/dsa/sja1105/sja1105_main.c
>> index 8b930cc2dabc..4b393782cc84 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_main.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
>> @@ -22,6 +22,7 @@
>> #include <linux/if_ether.h>
>> #include <linux/dsa/8021q.h>
>> #include "sja1105.h"
>> +#include "sja1105_tas.h"
>>
>> static void sja1105_hw_reset(struct gpio_desc *gpio, unsigned int
>> pulse_len,
>> unsigned int startup_delay)
>> @@ -1382,7 +1383,7 @@ static void sja1105_bridge_leave(struct dsa_switch
>> *ds, int port,
>> * modify at runtime (currently only MAC) and restore them after
>> uploading,
>> * such that this operation is relatively seamless.
>> */
>> -static int sja1105_static_config_reload(struct sja1105_private *priv)
>> +int sja1105_static_config_reload(struct sja1105_private *priv)
>> {
>> struct ptp_system_timestamp ptp_sts_before;
>> struct ptp_system_timestamp ptp_sts_after;
>> @@ -1761,6 +1762,7 @@ static void sja1105_teardown(struct dsa_switch *ds)
>> {
>> struct sja1105_private *priv = ds->priv;
>>
>> + sja1105_tas_teardown(priv);
>> cancel_work_sync(&priv->tagger_data.rxtstamp_work);
>> skb_queue_purge(&priv->tagger_data.skb_rxtstamp_queue);
>> sja1105_ptp_clock_unregister(priv);
>> @@ -2088,6 +2090,18 @@ static bool sja1105_port_txtstamp(struct dsa_switch
>> *ds, int port,
>> return true;
>> }
>>
>> +static int sja1105_port_setup_tc(struct dsa_switch *ds, int port,
>> + enum tc_setup_type type,
>> + void *type_data)
>> +{
>> + switch (type) {
>> + case TC_SETUP_QDISC_TAPRIO:
>> + return sja1105_setup_tc_taprio(ds, port, type_data);
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> static const struct dsa_switch_ops sja1105_switch_ops = {
>> .get_tag_protocol = sja1105_get_tag_protocol,
>> .setup = sja1105_setup,
>> @@ -2120,6 +2134,7 @@ static const struct dsa_switch_ops
>> sja1105_switch_ops = {
>> .port_hwtstamp_set = sja1105_hwtstamp_set,
>> .port_rxtstamp = sja1105_port_rxtstamp,
>> .port_txtstamp = sja1105_port_txtstamp,
>> + .port_setup_tc = sja1105_port_setup_tc,
>> };
>>
>> static int sja1105_check_device_id(struct sja1105_private *priv)
>> @@ -2229,6 +2244,8 @@ static int sja1105_probe(struct spi_device *spi)
>> }
>> mutex_init(&priv->mgmt_lock);
>>
>> + sja1105_tas_setup(priv);
>> +
>> return dsa_register_switch(priv->ds);
>> }
>>
>> diff --git a/drivers/net/dsa/sja1105/sja1105_tas.c
>> b/drivers/net/dsa/sja1105/sja1105_tas.c
>> new file mode 100644
>> index 000000000000..769e1d8e5e8f
>> --- /dev/null
>> +++ b/drivers/net/dsa/sja1105/sja1105_tas.c
>> @@ -0,0 +1,420 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
>> + */
>> +#include "sja1105.h"
>> +
>> +#define SJA1105_TAS_CLKSRC_DISABLED 0
>> +#define SJA1105_TAS_CLKSRC_STANDALONE 1
>> +#define SJA1105_TAS_CLKSRC_AS6802 2
>> +#define SJA1105_TAS_CLKSRC_PTP 3
>> +#define SJA1105_GATE_MASK GENMASK_ULL(SJA1105_NUM_TC - 1, 0)
>> +#define SJA1105_TAS_MAX_DELTA BIT(19)
>> +
>> +/* This is not a preprocessor macro because the "ns" argument may or may
>> not be
>> + * s64 at caller side. This ensures it is properly type-cast before
>> div_s64.
>> + */
>> +static s64 ns_to_sja1105_delta(s64 ns)
>> +{
>> + return div_s64(ns, 200);
>> +}
>> +
>> +/* Lo and behold: the egress scheduler from hell.
>> + *
>> + * At the hardware level, the Time-Aware Shaper holds a global linear
>> arrray of
>> + * all schedule entries for all ports. These are the Gate Control List
>> (GCL)
>> + * entries, let's call them "timeslots" for short. This linear array of
>> + * timeslots is held in BLK_IDX_SCHEDULE.
>> + *
>> + * Then there are a maximum of 8 "execution threads" inside the switch,
>> which
>> + * iterate cyclically through the "schedule". Each "cycle" has an entry
>> point
>> + * and an exit point, both being timeslot indices in the schedule table.
>> The
>> + * hardware calls each cycle a "subschedule".
>> + *
>> + * Subschedule (cycle) i starts when
>> + * ptpclkval >= ptpschtm + BLK_IDX_SCHEDULE_ENTRY_POINTS[i].delta.
>> + *
>> + * The hardware scheduler iterates BLK_IDX_SCHEDULE with a k ranging
>> from
>> + * k = BLK_IDX_SCHEDULE_ENTRY_POINTS[i].address to
>> + * k = BLK_IDX_SCHEDULE_PARAMS.subscheind[i]
>> + *
>> + * For each schedule entry (timeslot) k, the engine executes the gate
>> control
>> + * list entry for the duration of BLK_IDX_SCHEDULE[k].delta.
>> + *
>> + * +---------+
>> + * | | BLK_IDX_SCHEDULE_ENTRY_POINTS_PARAMS
>> + * +---------+
>> + * |
>> + * +-----------------+
>> + * | .actsubsch
>> + * BLK_IDX_SCHEDULE_ENTRY_POINTS v
>> + * +-------+-------+
>> + * |cycle 0|cycle 1|
>> + * +-------+-------+
>> + * | | | |
>> + * +----------------+ | |
>> +-------------------------------------+
>> + * | .subschindx | | .subschindx
>> |
>> + * | | +---------------+
>> |
>> + * | .address | .address |
>> |
>> + * | | |
>> |
>> + * | | |
>> |
>> + * | BLK_IDX_SCHEDULE v v
>> |
>> + * | +-------+-------+-------+-------+-------+------+
>> |
>> + * | |entry 0|entry 1|entry 2|entry 3|entry 4|entry5|
>> |
>> + * | +-------+-------+-------+-------+-------+------+
>> |
>> + * | ^ ^ ^ ^
>> |
>> + * | | | | |
>> |
>> + * | +-------------------------+ | | |
>> |
>> + * | | +-------------------------------+ | |
>> |
>> + * | | | +-------------------+ |
>> |
>> + * | | | | |
>> |
>> + * | +---------------------------------------------------------------+
>> |
>> + * | |subscheind[0]<=subscheind[1]<=subscheind[2]<=...<=subscheind[7]|
>> |
>> + * | +---------------------------------------------------------------+
>> |
>> + * | ^ ^ BLK_IDX_SCHEDULE_PARAMS
>> |
>> + * | | |
>> |
>> + * +--------+
>> +-------------------------------------------+
>> + *
>> + * In the above picture there are two subschedules (cycles):
>> + *
>> + * - cycle 0: iterates the schedule table from 0 to 2 (and back)
>> + * - cycle 1: iterates the schedule table from 3 to 5 (and back)
>> + *
>> + * All other possible execution threads must be marked as unused by
>> making
>> + * their "subschedule end index" (subscheind) equal to the last valid
>> + * subschedule's end index (in this case 5).
>> + */
>> +static int sja1105_init_scheduling(struct sja1105_private *priv)
>> +{
>> + struct sja1105_schedule_entry_points_entry *schedule_entry_points;
>> + struct sja1105_schedule_entry_points_params_entry
>> + *schedule_entry_points_params;
>> + struct sja1105_schedule_params_entry *schedule_params;
>> + struct sja1105_tas_data *tas_data = &priv->tas_data;
>> + struct sja1105_schedule_entry *schedule;
>> + struct sja1105_table *table;
>> + int subscheind[8] = {0};
>> + int schedule_start_idx;
>> + s64 entry_point_delta;
>> + int schedule_end_idx;
>> + int num_entries = 0;
>> + int num_cycles = 0;
>> + int cycle = 0;
>> + int i, k = 0;
>> + int port;
>> +
>> + /* Discard previous Schedule Table */
>> + table = &priv->static_config.tables[BLK_IDX_SCHEDULE];
>> + if (table->entry_count) {
>> + kfree(table->entries);
>> + table->entry_count = 0;
>> + }
>> +
>> + /* Discard previous Schedule Entry Points Parameters Table */
>> + table =
>> &priv->static_config.tables[BLK_IDX_SCHEDULE_ENTRY_POINTS_PARAMS];
>> + if (table->entry_count) {
>> + kfree(table->entries);
>> + table->entry_count = 0;
>> + }
>> +
>> + /* Discard previous Schedule Parameters Table */
>> + table = &priv->static_config.tables[BLK_IDX_SCHEDULE_PARAMS];
>> + if (table->entry_count) {
>> + kfree(table->entries);
>> + table->entry_count = 0;
>> + }
>> +
>> + /* Discard previous Schedule Entry Points Table */
>> + table = &priv->static_config.tables[BLK_IDX_SCHEDULE_ENTRY_POINTS];
>> + if (table->entry_count) {
>> + kfree(table->entries);
>> + table->entry_count = 0;
>> + }
>> +
>> + /* Figure out the dimensioning of the problem */
>> + for (port = 0; port < SJA1105_NUM_PORTS; port++) {
>> + if (tas_data->config[port]) {
>> + num_entries += tas_data->config[port]->num_entries;
>> + num_cycles++;
>> + }
>> + }
>> +
>> + /* Nothing to do */
>> + if (!num_cycles)
>> + return 0;
>> +
>> + /* Pre-allocate space in the static config tables */
>> +
>> + /* Schedule Table */
>> + table = &priv->static_config.tables[BLK_IDX_SCHEDULE];
>> + table->entries = kcalloc(num_entries, table->ops->unpacked_entry_size,
>> + GFP_KERNEL);
>> + if (!table->entries)
>> + return -ENOMEM;
>> + table->entry_count = num_entries;
>> + schedule = table->entries;
>> +
>> + /* Schedule Points Parameters Table */
>> + table =
>> &priv->static_config.tables[BLK_IDX_SCHEDULE_ENTRY_POINTS_PARAMS];
>> + table->entries =
>> kcalloc(SJA1105_MAX_SCHEDULE_ENTRY_POINTS_PARAMS_COUNT,
>> + table->ops->unpacked_entry_size, GFP_KERNEL);
>> + if (!table->entries)
>> + return -ENOMEM;
>
> Should this free the previous allocation, in case this one fails?
> (also applies to the statements below)
>
I had to take a look at the overall driver code again, since it's
already been a while since I added it and I couldn't remember exactly.
All memory is freed automagically in sja1105_static_config_free from
sja1105_static_config.c. That simplifies driver code considerably,
although it's so generic that I forgot that it's there.
Thanks,
-Vladimir
^ permalink raw reply
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local
From: maowenan @ 2019-09-12 2:05 UTC (permalink / raw)
To: Marcelo Ricardo Leitner, Dan Carpenter
Cc: vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <20190911143923.GE3499@localhost.localdomain>
On 2019/9/11 22:39, Marcelo Ricardo Leitner wrote:
> On Wed, Sep 11, 2019 at 11:30:08AM -0300, Marcelo Ricardo Leitner wrote:
>> On Wed, Sep 11, 2019 at 11:30:38AM +0300, Dan Carpenter wrote:
>>> On Wed, Sep 11, 2019 at 09:30:47AM +0800, maowenan wrote:
>>>>
>>>>
>>>> On 2019/9/11 3:22, Dan Carpenter wrote:
>>>>> On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote:
>>>>>> On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote:
>>>>>>> There are more parentheses in if clause when call sctp_get_port_local
>>>>>>> in sctp_do_bind, and redundant assignment to 'ret'. This patch is to
>>>>>>> do cleanup.
>>>>>>>
>>>>>>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>>>>>>> ---
>>>>>>> net/sctp/socket.c | 3 +--
>>>>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>>> index 9d1f83b10c0a..766b68b55ebe 100644
>>>>>>> --- a/net/sctp/socket.c
>>>>>>> +++ b/net/sctp/socket.c
>>>>>>> @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
>>>>>>> * detection.
>>>>>>> */
>>>>>>> addr->v4.sin_port = htons(snum);
>>>>>>> - if ((ret = sctp_get_port_local(sk, addr))) {
>>>>>>> + if (sctp_get_port_local(sk, addr))
>>>>>>> return -EADDRINUSE;
>>>>>>
>>>>>> sctp_get_port_local() returns a long which is either 0,1 or a pointer
>>>>>> casted to long. It's not documented what it means and neither of the
>>>>>> callers use the return since commit 62208f12451f ("net: sctp: simplify
>>>>>> sctp_get_port").
>>>>>
>>>>> Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening
>>>>> socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug,
>>>>> because before the code assumed that a pointer casted to an int was the
>>>>> same as a pointer casted to a long.
>>>>
>>>> commit 4e54064e0a13 treated non-zero return value as unexpected, so the current
>>>> cleanup is ok?
>>>
>>> Yeah. It's fine, I was just confused why we weren't preserving the
>>> error code and then I saw that we didn't return errors at all and got
>>> confused.
>>
>> But please lets seize the moment and do the change Dean suggested.
>
> *Dan*, sorry.
>
>> This was the last place saving this return value somewhere. It makes
>> sense to cleanup sctp_get_port_local() now and remove that masked
>> pointer return.
>>
>> Then you may also cleanup:
>> socket.c: return !!sctp_get_port_local(sk, &addr);
>> as it will be a direct map.
Thanks Marcelo, shall I post a new individual patch for cleanup as your suggest?
>>
>> Marcelo
>>
>
> .
>
^ permalink raw reply
* Klientskie Bazy http://prodawez.tilda.ws/page7270311.html
From: netdev @ 2019-09-12 1:18 UTC (permalink / raw)
To: netdev
Klientskie Bazy http://prodawez.tilda.ws/page7270311.html
^ permalink raw reply
* [PATCH v2 net 0/3] fix memory leak for sctp_do_bind
From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw)
To: vyasevich, nhorman, marcelo.leitner, davem
Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan
In-Reply-To: <7a450679-40ca-8a84-4cba-7a16f22ea3c0@huawei.com>
First two patches are to do cleanup, remove redundant assignment,
and change return type of sctp_get_port_local.
Third patch is to fix memory leak for sctp_do_bind if failed
to bind address.
---
v2: add one patch to change return type of sctp_get_port_local.
---
Mao Wenan (3):
sctp: change return type of sctp_get_port_local
sctp: remove redundant assignment when call sctp_get_port_local
sctp: destroy bucket if failed to bind addr
net/sctp/socket.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr
From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw)
To: vyasevich, nhorman, marcelo.leitner, davem
Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan,
Hulk Robot
In-Reply-To: <20190912040219.67517-1-maowenan@huawei.com>
There is one memory leak bug report:
BUG: memory leak
unreferenced object 0xffff8881dc4c5ec0 (size 40):
comm "syz-executor.0", pid 5673, jiffies 4298198457 (age 27.578s)
hex dump (first 32 bytes):
02 00 00 00 81 88 ff ff 00 00 00 00 00 00 00 00 ................
f8 63 3d c1 81 88 ff ff 00 00 00 00 00 00 00 00 .c=.............
backtrace:
[<0000000072006339>] sctp_get_port_local+0x2a1/0xa00 [sctp]
[<00000000c7b379ec>] sctp_do_bind+0x176/0x2c0 [sctp]
[<000000005be274a2>] sctp_bind+0x5a/0x80 [sctp]
[<00000000b66b4044>] inet6_bind+0x59/0xd0 [ipv6]
[<00000000c68c7f42>] __sys_bind+0x120/0x1f0 net/socket.c:1647
[<000000004513635b>] __do_sys_bind net/socket.c:1658 [inline]
[<000000004513635b>] __se_sys_bind net/socket.c:1656 [inline]
[<000000004513635b>] __x64_sys_bind+0x3e/0x50 net/socket.c:1656
[<0000000061f2501e>] do_syscall_64+0x72/0x2e0 arch/x86/entry/common.c:296
[<0000000003d1e05e>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
This is because in sctp_do_bind, if sctp_get_port_local is to
create hash bucket successfully, and sctp_add_bind_addr failed
to bind address, e.g return -ENOMEM, so memory leak found, it
needs to destroy allocated bucket.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Mao Wenan <maowenan@huawei.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
net/sctp/socket.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 2f810078c91d..69ec3b796197 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -412,11 +412,13 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len,
SCTP_ADDR_SRC, GFP_ATOMIC);
- /* Copy back into socket for getsockname() use. */
- if (!ret) {
- inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num);
- sp->pf->to_sk_saddr(addr, sk);
+ if (ret) {
+ sctp_put_port(sk);
+ return ret;
}
+ /* Copy back into socket for getsockname() use. */
+ inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num);
+ sp->pf->to_sk_saddr(addr, sk);
return ret;
}
--
2.20.1
^ permalink raw reply related
* [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local
From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw)
To: vyasevich, nhorman, marcelo.leitner, davem
Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan
In-Reply-To: <20190912040219.67517-1-maowenan@huawei.com>
There are more parentheses in if clause when call sctp_get_port_local
in sctp_do_bind, and redundant assignment to 'ret'. This patch is to
do cleanup.
Signed-off-by: Mao Wenan <maowenan@huawei.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
net/sctp/socket.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5e1934c48709..2f810078c91d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
* detection.
*/
addr->v4.sin_port = htons(snum);
- if ((ret = sctp_get_port_local(sk, addr))) {
+ if (sctp_get_port_local(sk, addr))
return -EADDRINUSE;
- }
/* Refresh ephemeral port. */
if (!bp->port)
--
2.20.1
^ permalink raw reply related
* [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local
From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw)
To: vyasevich, nhorman, marcelo.leitner, davem
Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan
In-Reply-To: <20190912040219.67517-1-maowenan@huawei.com>
Currently sctp_get_port_local() returns a long
which is either 0,1 or a pointer casted to long.
It's neither of the callers use the return value since
commit 62208f12451f ("net: sctp: simplify sctp_get_port").
Now two callers are sctp_get_port and sctp_do_bind,
they actually assumend a casted to an int was the same as
a pointer casted to a long, and they don't save the return
value just check whether it is zero or non-zero, so
it would better change return type from long to int for
sctp_get_port_local.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
net/sctp/socket.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9d1f83b10c0a..5e1934c48709 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -309,7 +309,7 @@ static int sctp_bind(struct sock *sk, struct sockaddr *addr, int addr_len)
return retval;
}
-static long sctp_get_port_local(struct sock *, union sctp_addr *);
+static int sctp_get_port_local(struct sock *, union sctp_addr *);
/* Verify this is a valid sockaddr. */
static struct sctp_af *sctp_sockaddr_af(struct sctp_sock *opt,
@@ -7998,7 +7998,7 @@ static void sctp_unhash(struct sock *sk)
static struct sctp_bind_bucket *sctp_bucket_create(
struct sctp_bind_hashbucket *head, struct net *, unsigned short snum);
-static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr)
+static int sctp_get_port_local(struct sock *sk, union sctp_addr *addr)
{
struct sctp_sock *sp = sctp_sk(sk);
bool reuse = (sk->sk_reuse || sp->reuse);
@@ -8108,7 +8108,7 @@ static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr)
if (sctp_bind_addr_conflict(&ep2->base.bind_addr,
addr, sp2, sp)) {
- ret = (long)sk2;
+ ret = 1;
goto fail_unlock;
}
}
@@ -8180,7 +8180,7 @@ static int sctp_get_port(struct sock *sk, unsigned short snum)
addr.v4.sin_port = htons(snum);
/* Note: sk->sk_num gets filled in if ephemeral port request. */
- return !!sctp_get_port_local(sk, &addr);
+ return sctp_get_port_local(sk, &addr);
}
/*
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net v2 01/11] net: core: limit nested device depth
From: Taehee Yoo @ 2019-09-12 3:56 UTC (permalink / raw)
To: David Miller
Cc: Netdev, j.vosburgh, vfalico, Andy Gospodarek,
Jiří Pírko, sd, Roopa Prabhu, saeedm, manishc,
rahulv, kys, haiyangz, sthemmin, sashal, hare, varun, ubraun,
kgraul, Jay Vosburgh
In-Reply-To: <20190912.003209.917226424625610557.davem@davemloft.net>
On Thu, 12 Sep 2019 at 07:32, David Miller <davem@davemloft.net> wrote:
>
Hi David
Thank you for the review!
> From: Taehee Yoo <ap420073@gmail.com>
> Date: Sat, 7 Sep 2019 22:45:32 +0900
>
> > Current code doesn't limit the number of nested devices.
> > Nested devices would be handled recursively and this needs huge stack
> > memory. So, unlimited nested devices could make stack overflow.
> ...
> > Splat looks like:
> > [ 140.483124] BUG: looking up invalid subclass: 8
> > [ 140.483505] turning off the locking correctness validator.
>
> The limit here is not stack memory, but a limit in the lockdep
> validator, which can probably be fixed by other means.
>
> This was the feedback I saw given for the previous version of
> this series as well.
I just realized this commit message is not enough for describing the problems.
It looks like that "invalid subclass" makes panic.
But this is not.
The panic is not related to "invalid subclass" lockdep problem.
There are two splats.
1. [ 140.483124] BUG: looking up invalid subclass: 8
2. [ 168.446539] BUG: KASAN: slab-out-of-bounds in __unwind_start+0x71/0x850
[ 168.794493] Rebooting in 5 seconds..
The first message is just a warning message of lockdep because of too deep
lockdep subclasses and it doesn't make any problem and panic.
This message can be ignored right now because other patches of
this patchset avoids this problem using dynamic lockdep key.
The second message is a panic message.
This is stack overflow and it occurs because of too deep nested devices.
The goal of this patch is to fix this stack overflow problem.
I tested with this reproducer commands without lockdep.
ip link add dummy0 type dummy
ip link add link dummy0 name vlan1 type vlan id 1
ip link set vlan1 up
for i in {2..200}
do
let A=$i-1
ip link add name vlan$i link vlan$A type vlan id $i
done
ip link del vlan1 <-- this command is added.
Splat looks like:
[ 181.594092] Thread overran stack, or stack corrupted
[ 181.594726] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
[ 181.595417] CPU: 1 PID: 1402 Comm: ip Not tainted 5.3.0-rc7+ #173
[ 181.596193] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[ 181.605986] RIP: 0010:stack_depot_fetch+0x10/0x30
[ 181.606588] Code: 00 75 10 48 8b 73 18 48 89 ef 5b 5d e9 59 bf 89
ff 0f 0b e8 02 3f 9d ff eb e9 89 f8 c1 ef 110
[ 181.609820] RSP: 0018:ffff8880cbebedd8 EFLAGS: 00010006
[ 181.610485] RAX: 00000000001fffff RBX: ffff8880cbebfc00 RCX: 0000000000000000
[ 181.611394] RDX: 000000000000001d RSI: ffff8880cbebede0 RDI: 0000000000003ff0
[ 181.612297] RBP: ffffea00032fae00 R08: ffffed101b5a3eab R09: ffffed101b5a3eab
[ 181.613222] R10: 0000000000000001 R11: ffffed101b5a3eaa R12: ffff8880d6115e80
[ 181.614148] R13: ffff8880cbebeac0 R14: ffff8880cbebfc00 R15: ffff8880cbebef80
[ 181.615053] FS: 00007f46140510c0(0000) GS:ffff8880dad00000(0000)
knlGS:0000000000000000
[ 181.616085] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 181.616841] CR2: ffffffffb9a7a0d8 CR3: 00000000bc356003 CR4: 00000000000606e0
[ 181.635748] Call Trace:
[ 181.635996] Modules linked in: 8021q garp stp mrp llc dummy veth
openvswitch nsh nf_conncount nf_nat nf_conntrs
[ 181.637360] CR2: ffffffffb9a7a0d8
[ 181.637670] ---[ end trace f890ce3e5c51ceb4 ]---
[ 181.638096] RIP: 0010:stack_depot_fetch+0x10/0x30
[ 181.638524] Code: 00 75 10 48 8b 73 18 48 89 ef 5b 5d e9 59 bf 89
ff 0f 0b e8 02 3f 9d ff eb e9 89 f8 c1 ef 110
[ 181.805441] RSP: 0018:ffff8880cbebedd8 EFLAGS: 00010006
[ 181.900192] RAX: 00000000001fffff RBX: ffff8880cbebfc00 RCX: 0000000000000000
[ 181.901119] RDX: 000000000000001d RSI: ffff8880cbebede0 RDI: 0000000000003ff0
[ 181.902038] RBP: ffffea00032fae00 R08: ffffed101b5a3eab R09: ffffed101b5a3eab
[ 181.902960] R10: 0000000000000001 R11: ffffed101b5a3eaa R12: ffff8880d6115e80
[ 181.903885] R13: ffff8880cbebeac0 R14: ffff8880cbebfc00 R15: ffff8880cbebef80
[ 181.904825] FS: 00007f46140510c0(0000) GS:ffff8880dad00000(0000)
knlGS:0000000000000000
[ 181.905862] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 181.906604] CR2: ffffffffb9a7a0d8 CR3: 00000000bc356003 CR4: 00000000000606e0
[ 181.907525] Kernel panic - not syncing: Fatal exception
[ 181.908179] Kernel Offset: 0x34000000 from 0xffffffff81000000
(relocation range: 0xffffffff80000000-0xffffffff)
[ 181.909176] Rebooting in 5 seconds..
^ permalink raw reply
* Re: [PATCH bpf-next v10 2/4] bpf: new helper to obtain namespace data from current task New bpf helper bpf_get_current_pidns_info.
From: Yonghong Song @ 2019-09-12 5:49 UTC (permalink / raw)
To: Eric W. Biederman, Al Viro
Cc: Carlos Antonio Neira Bustos, netdev@vger.kernel.org,
brouer@redhat.com, bpf@vger.kernel.org
In-Reply-To: <87o8zr8cz3.fsf@x220.int.ebiederm.org>
On 9/11/19 9:16 AM, Eric W. Biederman wrote:
> Al Viro <viro@zeniv.linux.org.uk> writes:
>
>> On Tue, Sep 10, 2019 at 10:35:09PM +0000, Yonghong Song wrote:
>>>
>>> Carlos,
>>>
>>> Discussed with Eric today for what is the best way to get
>>> the device number for a namespace. The following patch seems
>>> a reasonable start although Eric would like to see
>>> how the helper is used in order to decide whether the
>>> interface looks right.
>>>
>>> commit bb00fc36d5d263047a8bceb3e51e969d7fbce7db (HEAD -> fs2)
>>> Author: Yonghong Song <yhs@fb.com>
>>> Date: Mon Sep 9 21:50:51 2019 -0700
>>>
>>> nsfs: add an interface function ns_get_inum_dev()
>>>
>>> This patch added an interface function
>>> ns_get_inum_dev(). Given a ns_common structure,
>>> the function returns the inode and device
>>> numbers. The function will be used later
>>> by a newly added bpf helper.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>
>>> diff --git a/fs/nsfs.c b/fs/nsfs.c
>>> index a0431642c6b5..a603c6fc3f54 100644
>>> --- a/fs/nsfs.c
>>> +++ b/fs/nsfs.c
>>> @@ -245,6 +245,14 @@ struct file *proc_ns_fget(int fd)
>>> return ERR_PTR(-EINVAL);
>>> }
>>>
>>> +/* Get the device number for the current task pidns.
>>> + */
>>> +void ns_get_inum_dev(struct ns_common *ns, u32 *inum, dev_t *dev)
>>> +{
>>> + *inum = ns->inum;
>>> + *dev = nsfs_mnt->mnt_sb->s_dev;
>>> +}
>>
>> Umm... Where would it get the device number once we get (hell knows
>> what for) multiple nsfs instances? I still don't understand what
>> would that be about, TBH... Is it really per-userns? Or something
>> else entirely? Eric, could you give some context?
>
> My goal is not to paint things into a corner, with future changes.
> Right now it is possible to stat a namespace file descriptor and
> get a device and inode number. Then compare that.
>
> I don't want people using the inode number in nsfd as some magic
> namespace id.
>
> We have had times in the past where there was more than one superblock
> and thus more than one device number. Further if userspace ever uses
> this heavily there may be times in the future where for
> checkpoint/restart purposes we will want multiple nsfd's so we can
> preserve the inode number accross a migration.
>
> Realistically there will probably just some kind of hotplug notification
> to userspace to say we have hotplugged your operatining system as
> a migration notification.
>
> Now the halway discussion did not quite capture everything I was trying
> to say but it at least got to the right ballpark.
>
> The helper in fs/nsfs.c should be:
>
> bool ns_match(const struct ns_common *ns, dev_t dev, ino_t ino)
> {
> return ((ns->inum == ino) && (nsfs_mnt->mnt_sb->s_dev == dev));
> }
>
> That way if/when there are multiple inodes identifying the same
> namespace the bpf programs don't need to change.
Thanks, Eric. This is indeed better. The bpf helper should focus
on comparing dev/ino, instead of return the dev/ino to bpf program.
So overall, nsfs related change will look like:
diff --git a/fs/nsfs.c b/fs/nsfs.c
index a0431642c6b5..7e78d89c2172 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -245,6 +245,11 @@ struct file *proc_ns_fget(int fd)
return ERR_PTR(-EINVAL);
}
+bool ns_match(const struct ns_common *ns, dev_t dev, ino_t ino)
+{
+ return ((ns->inum == ino) && (nsfs_mnt->mnt_sb->s_dev == dev));
+}
+
static int nsfs_show_path(struct seq_file *seq, struct dentry *dentry)
{
struct inode *inode = d_inode(dentry);
diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index d31cb6215905..79639807e960 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h
@@ -81,6 +81,7 @@ extern void *ns_get_path(struct path *path, struct
task_struct *task,
typedef struct ns_common *ns_get_path_helper_t(void *);
extern void *ns_get_path_cb(struct path *path, ns_get_path_helper_t
ns_get_cb,
void *private_data);
+extern bool ns_match(const struct ns_common *ns, dev_t dev, ino_t ino);
extern int ns_get_name(char *buf, size_t size, struct task_struct *task,
const struct proc_ns_operations *ns_ops);
>
> Up farther in the stack it should be something like:
>
>> BPF_CALL_2(bpf_current_pidns_match, dev_t *dev, ino_t *ino)
>> {
>> return ns_match(&task_active_pid_ns(current)->ns, *dev, *ino);
>> }
>>
>> const struct bpf_func_proto bpf_current_pidns_match_proto = {
>> .func = bpf_current_pins_match,
>> .gpl_only = true,
>> .ret_type = RET_INTEGER
>> .arg1_type = ARG_PTR_TO_DEVICE_NUMBER,
>> .arg2_type = ARG_PTR_TO_INODE_NUMBER,
>> };
>
> That allows comparing what the bpf came up with with whatever value
> userspace generated by stating the file descriptor.
>
>
> That is the least bad suggestion I currently have for that
> functionality. It really would be better to not have that filter in the
> bpf program itself but in the infrastructure that binds a program to a
> set of tasks.
>
> The problem with this approach is whatever device/inode you have when
> the namespace they refer to exits there is the possibility that the
> inode will be reused. So your filter will eventually start matching on
> the wrong thing.
I come up with a differeent helper definition, which is much more
similar to existing bpf_get_current_pid_tgid() and helper definition
much more conforms to bpf convention.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 5e28718928ca..bc26903c80c7 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -11,6 +11,8 @@
#include <linux/uidgid.h>
#include <linux/filter.h>
#include <linux/ctype.h>
+#include <linux/pid_namespace.h>
+#include <linux/proc_ns.h>
#include "../../lib/kstrtox.h"
@@ -487,3 +489,33 @@ const struct bpf_func_proto bpf_strtoul_proto = {
.arg4_type = ARG_PTR_TO_LONG,
};
#endif
+
+BPF_CALL_2(bpf_get_ns_current_pid_tgid, u32, dev, u32, inum)
+{
+ struct task_struct *task = current;
+ struct pid_namespace *pidns;
+ pid_t pid, tgid;
+
+ if (unlikely(!task))
+ return -EINVAL;
+
+
+ pidns = task_active_pid_ns(task);
+ if (unlikely(!pidns))
+ return -ENOENT;
+
+ if (!ns_match(&pidns->ns, (dev_t)dev, inum))
+ return -EINVAL;
+
+ pid = task_pid_nr_ns(task, pidns);
+ tgid = task_tgid_nr_ns(task, pidns);
+
+ return (u64) tgid << 32 | pid;
+}
+
+const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto = {
+ .func = bpf_get_ns_current_pid_tgid,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_ANYTHING,
+ .arg2_type = ARG_ANYTHING,
+};
Existing usage of bpf_get_current_pid_tgid() can be converted
to bpf_get_ns_current_pid_tgid() if ns dev/inode number
is supplied. For bpf_get_ns_current_pid_tgid(), checking
return value ( < 0 or not) is needed.
>
> Eric
>
^ permalink raw reply related
* Re: [PATCH V2 net-next 1/7] net: hns3: add ethtool_ops.set_channels support for HNS3 VF driver
From: Michal Kubecek @ 2019-09-12 6:23 UTC (permalink / raw)
To: netdev
Cc: Huazhong Tan, davem, linux-kernel, salil.mehta, yisen.zhuang,
linuxarm, jakub.kicinski
In-Reply-To: <1568169639-43658-2-git-send-email-tanhuazhong@huawei.com>
On Wed, Sep 11, 2019 at 10:40:33AM +0800, Huazhong Tan wrote:
> From: Guangbin Huang <huangguangbin2@huawei.com>
>
> This patch adds ethtool_ops.set_channels support for HNS3 VF driver,
> and updates related TQP information and RSS information, to support
> modification of VF TQP number, and uses current rss_size instead of
> max_rss_size to initialize RSS.
>
> Also, fixes a format error in hclgevf_get_rss().
>
> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 1 +
> .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 83 ++++++++++++++++++++--
> 2 files changed, 79 insertions(+), 5 deletions(-)
>
...
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> index 594cae8..e3090b3 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
...
> +static void hclgevf_update_rss_size(struct hnae3_handle *handle,
> + u32 new_tqps_num)
> +{
> + struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> + struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
> + u16 max_rss_size;
> +
> + kinfo->req_rss_size = new_tqps_num;
> +
> + max_rss_size = min_t(u16, hdev->rss_size_max,
> + hdev->num_tqps / kinfo->num_tc);
> +
> + /* Use the user's configuration when it is not larger than
> + * max_rss_size, otherwise, use the maximum specification value.
> + */
> + if (kinfo->req_rss_size != kinfo->rss_size && kinfo->req_rss_size &&
> + kinfo->req_rss_size <= max_rss_size)
> + kinfo->rss_size = kinfo->req_rss_size;
> + else if (kinfo->rss_size > max_rss_size ||
> + (!kinfo->req_rss_size && kinfo->rss_size < max_rss_size))
> + kinfo->rss_size = max_rss_size;
I don't think requested channel count can be larger than max_rss_size
here. In ethtool_set_channels(), we check that requested channel counts
do not exceed maximum channel counts as reported by ->get_channels().
And hclgevf_get_max_channels() cannot return more than max_rss_size.
> +
> + kinfo->num_tqps = kinfo->num_tc * kinfo->rss_size;
> +}
> +
> +static int hclgevf_set_channels(struct hnae3_handle *handle, u32 new_tqps_num,
> + bool rxfh_configured)
> +{
> + struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
> + struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> + u16 cur_rss_size = kinfo->rss_size;
> + u16 cur_tqps = kinfo->num_tqps;
> + u32 *rss_indir;
> + unsigned int i;
> + int ret;
> +
> + hclgevf_update_rss_size(handle, new_tqps_num);
> +
> + ret = hclgevf_set_rss_tc_mode(hdev, kinfo->rss_size);
> + if (ret)
> + return ret;
> +
> + /* RSS indirection table has been configuared by user */
> + if (rxfh_configured)
> + goto out;
> +
> + /* Reinitializes the rss indirect table according to the new RSS size */
> + rss_indir = kcalloc(HCLGEVF_RSS_IND_TBL_SIZE, sizeof(u32), GFP_KERNEL);
> + if (!rss_indir)
> + return -ENOMEM;
> +
> + for (i = 0; i < HCLGEVF_RSS_IND_TBL_SIZE; i++)
> + rss_indir[i] = i % kinfo->rss_size;
> +
> + ret = hclgevf_set_rss(handle, rss_indir, NULL, 0);
> + if (ret)
> + dev_err(&hdev->pdev->dev, "set rss indir table fail, ret=%d\n",
> + ret);
> +
> + kfree(rss_indir);
> +
> +out:
> + if (!ret)
> + dev_info(&hdev->pdev->dev,
> + "Channels changed, rss_size from %u to %u, tqps from %u to %u",
> + cur_rss_size, kinfo->rss_size,
> + cur_tqps, kinfo->rss_size * kinfo->num_tc);
> +
> + return ret;
> +}
IIRC David asked you not to issue this log message in v1 review.
Michal Kubecek
^ permalink raw reply
* Re: VRF Issue Since kernel 5
From: Gowen @ 2019-09-12 6:50 UTC (permalink / raw)
To: David Ahern, netdev@vger.kernel.org
In-Reply-To: <b300235a-00f8-0689-8544-9db07cbd1e21@gmail.com>
Hi David -thanks for getting back to me
The DNS servers are 10.24.65.203 or 10.24.64.203 which you want to go
out mgmt-vrf. correct? No - 10.24.65.203 10.25.65.203, so should hit the route leak rule as below (if I've put the 10.24.64.0/24 subnet anywhere it is a typo)
vmAdmin@NETM06:~$ ip ro get 10.24.65.203 fibmatch
10.24.65.0/24 via 10.24.12.1 dev eth0
I've added the 127/8 route - no difference.
The reason for what you might think is an odd design is that I wanted any non-VRF aware users to be able to come in and run all commands in default context without issue, while production and mgmt traffic was separated still
DNS is now working as long as /etc/resolv.conf is populated with my DNS servers - a lot of people would be using this on Azure which uses netplan, so they'll have the same issue, is there documentation I could update or raise a bug to check the systemd-resolve servers as well?
Gareth
From: David Ahern <dsahern@gmail.com>
Sent: 11 September 2019 18:02
To: Gowen <gowen@potatocomputing.co.uk>; netdev@vger.kernel.org <netdev@vger.kernel.org>
Subject: Re: VRF Issue Since kernel 5
At LPC this week and just now getting a chance to process the data you sent.
On 9/9/19 8:46 AM, Gowen wrote:
> the production traffic is all in the 10.0.0.0/8 network (eth1 global VRF) except for a few subnets (DNS) which are routed out eth0 (mgmt-vrf)
>
>
> Admin@NETM06:~$ ip route show
> default via 10.24.12.1 dev eth0
> 10.0.0.0/8 via 10.24.12.1 dev eth1
> 10.24.12.0/24 dev eth1 proto kernel scope link src 10.24.12.9
> 10.24.65.0/24 via 10.24.12.1 dev eth0
> 10.25.65.0/24 via 10.24.12.1 dev eth0
> 10.26.0.0/21 via 10.24.12.1 dev eth0
> 10.26.64.0/21 via 10.24.12.1 dev eth0
interesting route table. This is default VRF but you have route leaking
through eth0 which is in mgmt-vrf.
>
>
> Admin@NETM06:~$ ip route show vrf mgmt-vrf
> default via 10.24.12.1 dev eth0
> unreachable default metric 4278198272
> 10.24.12.0/24 dev eth0 proto kernel scope link src 10.24.12.10
> 10.24.65.0/24 via 10.24.12.1 dev eth0
> 10.25.65.0/24 via 10.24.12.1 dev eth0
> 10.26.0.0/21 via 10.24.12.1 dev eth0
> 10.26.64.0/21 via 10.24.12.1 dev eth0
The DNS servers are 10.24.65.203 or 10.24.64.203 which you want to go
out mgmt-vrf. correct?
10.24.65.203 should hit the route "10.24.65.0/24 via 10.24.12.1 dev
eth0" for both default VRF and mgmt-vrf.
10.24.64.203 will NOT hit a route leak entry so traverse the VRF
associated with the context of the command (mgmt-vrf or default). Is
that intentional? (verify with: `ip ro get 10.24.64.203 fibmatch` and
`ip ro get 10.24.64.203 vrf mgmt-vrf fibmatch`)
>
>
>
> The strange activity occurs when I enter the command “sudo apt update” as I can resolve the DNS request (10.24.65.203 or 10.24.64.203, verified with tcpdump) out eth0 but for the actual update traffic there is no activity:
>
>
> sudo tcpdump -i eth0 '(host 10.24.65.203 or host 10.25.65.203) and port 53' -n
> <OUTPUT OMITTED FOR BREVITY>
> 10:06:05.268735 IP 10.24.12.10.39963 > 10.24.65.203.53: 48798+ [1au] A? security.ubuntu.com. (48)
> <OUTPUT OMITTED FOR BREVITY>
> 10:06:05.284403 IP 10.24.65.203.53 > 10.24.12.10.39963: 48798 13/0/1 A 91.189.91.23, A 91.189.88.24, A 91.189.91.26, A 91.189.88.162, A 91.189.88.149, A 91.189.91.24, A 91.189.88.173, A 91.189.88.177, A 91.189.88.31, A 91.189.91.14, A 91.189.88.176, A 91.189.88.175,
A 91.189.88.174 (256)
>
>
>
> You can see that the update traffic is returned but is not accepted by the stack and a RST is sent
>
>
> Admin@NETM06:~$ sudo tcpdump -i eth0 '(not host 168.63.129.16 and port 80)' -n
> tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
> listening on eth0, link-type EN10MB (Ethernet), capture size 262144 bytes
> 10:17:12.690658 IP 10.24.12.10.40216 > 91.189.88.175.80: Flags [S], seq 2279624826, win 64240, options [mss 1460,sackOK,TS val 2029365856 ecr 0,nop,wscale 7], length 0
> 10:17:12.691929 IP 10.24.12.10.52362 > 91.189.95.83.80: Flags [S], seq 1465797256, win 64240, options [mss 1460,sackOK,TS val 3833463674 ecr 0,nop,wscale 7], length 0
> 10:17:12.696270 IP 91.189.88.175.80 > 10.24.12.10.40216: Flags [S.], seq 968450722, ack 2279624827, win 28960, options [mss 1418,sackOK,TS val 81957103 ecr 2029365856,nop,wscale 7], length 0
> 10:17:12.696301 IP 10.24.12.10.40216 > 91.189.88.175.80: Flags [R], seq 2279624827, win 0, length 0
> 10:17:12.697884 IP 91.189.95.83.80 > 10.24.12.10.52362: Flags [S.], seq 4148330738, ack 1465797257, win 28960, options [mss 1418,sackOK,TS val 2257624414 ecr 3833463674,nop,wscale 8], length 0
> 10:17:12.697909 IP 10.24.12.10.52362 > 91.189.95.83.80: Flags [R], seq 1465797257, win 0, length 0
>
>
>
>
> I can emulate the DNS lookup using netcat in the vrf:
>
>
> sudo ip vrf exec mgmt-vrf nc -u 10.24.65.203 53
>
`ip vrf exec mgmt-vrf <COMMAND>` means that every IPv4 and IPv6 socket
opened by <COMMAND> is automatically bound to mgmt-vrf which causes
route lookups to hit the mgmt-vrf table.
Just running <COMMAND> (without binding to any vrf) means no socket is
bound to anything unless the command does a bind. In that case the
routing lookups determine which egress device is used.
Now the response comes back, if the ingress interface is a VRF then the
socket lookup wants to match on a device.
Now, a later response shows this for DNS lookups:
isc-worker0000 20261 [000] 2215.013849: fib:fib_table_lookup: table
10 oif 0 iif 0 proto 0 0.0.0.0/0 -> 127.0.0.1/0 tos 0 scope 0 flags 0
==> dev eth0 gw 10.24.12.1 src 10.24.12.10 err 0
isc-worker0000 20261 [000] 2215.013915: fib:fib_table_lookup: table
10 oif 4 iif 1 proto 17 0.0.0.0/52138 -> 127.0.0.53/53 tos 0 scope 0
flags 4 ==> dev eth0 gw 10.24.12.1 src 10.24.12.10 err 0
isc-worker0000 20261 [000] 2220.014006: fib:fib_table_lookup: table
10 oif 4 iif 1 proto 17 0.0.0.0/52138 -> 127.0.0.53/53 tos 0 scope 0
flags 4 ==> dev eth0 gw 10.24.12.1 src 10.24.12.10 err 0
which suggests your process is passing off the DNS lookup to a local
process (isc-worker) and it hits the default route for mgmt-vrf when it
is trying to connect to a localhost address.
For mgmt-vrf I suggest always adding 127.0.0.1/8 to the mgmt vrf device
(and ::1/128 for IPv6 starting with 5.x kernels - I forget the exact
kernel version).
That might solve your problem; it might not.
(BTW: Cumulus uses fib rules for DNS servers to force DNS packets out
the mgmt-vrf interface.)
^ 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