* [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error
From: Nazarov Sergey @ 2019-02-25 16:27 UTC (permalink / raw)
To: David Miller
Cc: paul@paul-moore.com, netdev@vger.kernel.org,
linux-security-module@vger.kernel.org, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org
In-Reply-To: <20190224.173328.1032826011262803545.davem@davemloft.net>
Extract IP options in cipso_v4_error and use __icmp_send.
Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
---
include/net/ip.h | 2 ++
net/ipv4/cipso_ipv4.c | 17 +++++++++++++++--
net/ipv4/ip_options.c | 22 +++++++++++++++++-----
3 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/include/net/ip.h b/include/net/ip.h
index 8866bfc..f0e8d06 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -667,6 +667,8 @@ static inline int ip_options_echo(struct net *net, struct ip_options *dopt,
}
void ip_options_fragment(struct sk_buff *skb);
+int __ip_options_compile(struct net *net, struct ip_options *opt,
+ struct sk_buff *skb, __be32 *info);
int ip_options_compile(struct net *net, struct ip_options *opt,
struct sk_buff *skb);
int ip_options_get(struct net *net, struct ip_options_rcu **optp,
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b..eff86a7 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1735,13 +1735,26 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option)
*/
void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
{
+ unsigned char optbuf[sizeof(struct ip_options) + 40];
+ struct ip_options *opt = (struct ip_options *)optbuf;
+
if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
return;
+ /*
+ * We might be called above the IP layer,
+ * so we can not use icmp_send and IPCB here.
+ */
+
+ memset(opt, 0, sizeof(struct ip_options));
+ opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
+ if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
+ return;
+
if (gateway)
- icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
+ __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
else
- icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
+ __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
}
/**
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index ed194d4..32a3504 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -251,8 +251,9 @@ static void spec_dst_fill(__be32 *spec_dst, struct sk_buff *skb)
* If opt == NULL, then skb->data should point to IP header.
*/
-int ip_options_compile(struct net *net,
- struct ip_options *opt, struct sk_buff *skb)
+int __ip_options_compile(struct net *net,
+ struct ip_options *opt, struct sk_buff *skb,
+ __be32 *info)
{
__be32 spec_dst = htonl(INADDR_ANY);
unsigned char *pp_ptr = NULL;
@@ -468,11 +469,22 @@ int ip_options_compile(struct net *net,
return 0;
error:
- if (skb) {
- icmp_send(skb, ICMP_PARAMETERPROB, 0, htonl((pp_ptr-iph)<<24));
- }
+ if (info)
+ *info = htonl((pp_ptr-iph)<<24);
return -EINVAL;
}
+
+int ip_options_compile(struct net *net,
+ struct ip_options *opt, struct sk_buff *skb)
+{
+ int ret;
+ __be32 info;
+
+ ret = __ip_options_compile(net, opt, skb, &info);
+ if (ret != 0 && skb)
+ icmp_send(skb, ICMP_PARAMETERPROB, 0, info);
+ return ret;
+}
EXPORT_SYMBOL(ip_options_compile);
/*
---
^ permalink raw reply related
* Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled
From: Eric Dumazet @ 2019-02-25 16:39 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, Stefano Brivio, David S . Miller, David Ahern
In-Reply-To: <20190225081759.GP10051@dhcp-12-139.nay.redhat.com>
On 02/25/2019 12:17 AM, Hangbin Liu wrote:
> On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote:
>>
>>
>> On 02/24/2019 08:12 PM, Hangbin Liu wrote:
>>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
>>> at running time.
>>>
>>
>>
>> Why is it better exactly ?
>>
>> IPv6 can be enabled on the host, but disabled per device
>>
>> /proc/sys/net/ipv6/conf/{name}/disable_ipv6
>
> Sorry, it looks I didn't make it clear in the commit description.
> This issue only occurs when IPv6 is disabled at boot time as there is
> no IPv6 route entry. Disable ipv6 on specific interface is not affected.
> So check ipv6_mod_enabled() is enough and we don't need to worry about
> the rcu_read_lock or the dev status.
>
> Should I update the commit description?
Certainly. Are you telling us skb->dev could be NULL here ?
Because rcu_read_lock() should already be asserted.
^ permalink raw reply
* [PATCH net-next] net: marvell: neta: disable comphy when setting mode
From: Marek Behún @ 2019-02-25 16:43 UTC (permalink / raw)
To: netdev; +Cc: Russell King, Marek Behún
The comphy driver for Armada 3700 by Miquèl Raynal (which is currently
in linux-next) does not actually set comphy mode when phy_set_mode_ext
is called. The mode is set at next call of phy_power_on.
Update the driver to semantics similar to mvpp2: helper
mvneta_comphy_init sets comphy mode and powers it on.
When mode is to be changed in mvneta_mac_config, first power the comphy
off, then call mvneta_comphy_init (which sets the mode to new one).
Only do this when new mode is different from old mode.
This should also work for Armada 38x, since in that comphy driver
methods power_on and power_off are unimplemented.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
drivers/net/ethernet/marvell/mvneta.c | 28 ++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ff275780040f..c0a3718b2e2a 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3149,11 +3149,26 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
return 0;
}
+static int mvneta_comphy_init(struct mvneta_port *pp)
+{
+ int ret;
+
+ if (!pp->comphy)
+ return 0;
+
+ ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
+ pp->phy_interface);
+ if (ret)
+ return ret;
+
+ return phy_power_on(pp->comphy);
+}
+
static void mvneta_start_dev(struct mvneta_port *pp)
{
int cpu;
- WARN_ON(phy_power_on(pp->comphy));
+ WARN_ON(mvneta_comphy_init(pp));
mvneta_max_rx_size_set(pp, pp->pkt_size);
mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
@@ -3525,12 +3540,15 @@ static void mvneta_mac_config(struct net_device *ndev, unsigned int mode,
if (state->speed == SPEED_2500)
new_ctrl4 |= MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE;
- if (pp->comphy &&
+ if (pp->comphy && pp->phy_interface != state->interface &&
(state->interface == PHY_INTERFACE_MODE_SGMII ||
state->interface == PHY_INTERFACE_MODE_1000BASEX ||
- state->interface == PHY_INTERFACE_MODE_2500BASEX))
- WARN_ON(phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
- state->interface));
+ state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
+ pp->phy_interface = state->interface;
+
+ WARN_ON(phy_power_off(pp->comphy));
+ WARN_ON(mvneta_comphy_init(pp));
+ }
if (new_ctrl0 != gmac_ctrl0)
mvreg_write(pp, MVNETA_GMAC_CTRL_0, new_ctrl0);
--
2.19.2
^ permalink raw reply related
* Re: [net-next 05/14] i40e: fix up 32 bit timespec references
From: Jesse Brandeburg @ 2019-02-25 16:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jeff Kirsher, David Miller, Networking, nhorman, sassmann,
jogreene, jacob.e.keller
In-Reply-To: <CAK8P3a3fSofgDdLZ3S+MaEVz8DHnX6HO6xGiRWfCjQ614Kf4Cg@mail.gmail.com>
On Mon, 25 Feb 2019 15:46:18 +0100
Arnd Bergmann <arnd@arndb.de> wrote:
> I just stumbled over code added by this older patch, and can't make sense
> of the commit description here. Was this an attempt to fix a bug, or
> just a cleanup?
Thanks Arnd for mentioning/posting this! This was an attempt at a clean
up, unfortunately I made a mistake which introduced a bug.
> > - then = ns_to_timespec64(delta);
> > mutex_lock(&pf->tmreg_lock);
> >
> > i40e_ptp_read(pf, &now);
> > - now = timespec64_add(now, then);
> > + timespec64_add_ns(&now, delta);
> > i40e_ptp_write(pf, (const struct timespec64 *)&now);
>
> The problem I noticed here is that 'delta' is a user provided 64-bit
> number from clock_adjtime(), and timespec64_add_ns() performs uses
> a repeated addition instead of a div/mod pair. When the number
> is large, we may end up adding a single second 8 billion times,
> which may take a while even on a fast CPU.
>
> Should the commit 0ac30ce43323 ("i40e: fix up 32 bit timespec
> references") just be reverted?
Interestingly, we just were discussing this fix this past Friday, and
have a patch internally to fix the issue correctly. Either way it
is a commit (revert or fix) so we might as well just send the fix ASAP.
Our team should have something posted to intel-wired-lan/netdev shortly,
and we'll get the patch officially to netdev after a brief test.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 4/9] bpf: add bpf helper bpf_skb_ecn_set_ce
From: Eric Dumazet @ 2019-02-25 16:52 UTC (permalink / raw)
To: Daniel Borkmann, Martin Lau
Cc: Lawrence Brakmo, netdev, Alexei Starovoitov, Kernel Team
In-Reply-To: <c9ebe42a-4fc5-74f5-1b80-30ec45772174@iogearbox.net>
On 02/25/2019 02:10 AM, Daniel Borkmann wrote:
> My understanding is that before doing any writes into skb, we should make
> sure the data area is private to us (and offset in linear data). In tc BPF
> (ingress, egress) we use bpf_try_make_writable() helper for this, others
> like act_{pedit,skbmod} or ovs have similar logic before writing into skb,
> note that in all these cases it's mostly about generic writes, so location
> could also be L4, for example.
>
> Difference of above helper compared to net/sched/sch_*.c instances could
> be that it's i) for the qdisc case it's only on egress INET_ECN_set_ce()
> and that there may be a convention that qdiscs specifically may mangle
> it whereas the helper could be called on ingress and egress and confuse
> other subsystems since they won't see original or race by seeing partially
> updated (invalid) packet.
>
> Eric, have a chance to clarify? Perhaps then would make sense to disallow
> the helper in cgroup ingress path.
Good observations Daniel, thanks for bringing this up.
skb_ensure_writable() seems a big hammer for the case we change some bits in IP header.
TCP cloned packets certainly can have their headers mangled, so maybe
we need to use something using skb_header_cloned() instead of skb_cloned()
^ permalink raw reply
* Re: INFO: task hung in rtnetlink_rcv_msg
From: Joe Perches @ 2019-02-25 16:55 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Daniel Borkmann
Cc: Jakub Kicinski, syzbot, ast, christian, davem, dsahern, hawk,
idosch, john.fastabend, kafai, ktkhai, linux-kernel, netdev,
petrm, roopa, songliubraving, syzkaller-bugs, xdp-newbies, yhs
In-Reply-To: <20190225093933.748ceab4@carbon>
On Mon, 2019-02-25 at 09:39 +0100, Jesper Dangaard Brouer wrote:
> On Sat, 23 Feb 2019 01:47:00 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> > On 02/22/2019 10:45 PM, Jakub Kicinski wrote:
> > > On Fri, 22 Feb 2019 12:14:57 -0800, Joe Perches wrote:
> > > > On Fri, 2019-02-22 at 12:01 -0800, Jakub Kicinski wrote:
> > > > > Hi!
> > > > >
> > > > > Seems like something funny is going on with get_maintainer.pl since XDP
> > > > > entry got added. We seem to have been CCed on:
> > > >
> > > > I suggest removing the MAINTAINERS line with
> > > >
> > > > K: xdp
> > > >
> > > > as xdp is a pretty generic file/patch content
> > > > regex match for the K: type
> > > >
> > > > $ git grep --name-only xdp | wc -l
> > > > 236
>
> I'm unsure how K: works, but you grep definitely selects some wrong files.
>
> I tried with "xdp_":
> git grep --name-only xdp_
>
> That does catch all the driver that have XDP support, which is the
> point of the exercise (to catch drivers).
>
> It does contain a couple of false-positives:
> drivers/net/ethernet/neterion/vxge/vxge-traffic.c
> drivers/thunderbolt/tb_msgs.h
> drivers/thunderbolt/xdomain.c
> sound/soc/codecs/rt5670.c
>
> Via the pattern '[^a-z]xdp_' I'm only left with the thunderbolt
> false-positive, as it have a data struct's called tb_xdp_*.
Then it's probably better to use
K: \Wxdp_
and if XDP_ is also to be matched, maybe
K: (?i:\Wxdp_)
^ permalink raw reply
* Re: [PATCH net] geneve: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Ahern @ 2019-02-25 16:57 UTC (permalink / raw)
To: Hangbin Liu, netdev; +Cc: Stefano Brivio, David S . Miller
In-Reply-To: <20190225041350.9913-1-liuhangbin@gmail.com>
On 2/24/19 9:13 PM, Hangbin Liu wrote:
> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
> at running time.
>
> Fixes: c0a47e44c098 ("geneve: should not call rt6_lookup() when ipv6 was disabled")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> drivers/net/geneve.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 3377ac66a347..42c5e43ece68 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1514,7 +1514,7 @@ static void geneve_link_config(struct net_device *dev,
> case AF_INET6: {
> struct rt6_info *rt;
>
> - if (!__in6_dev_get(dev))
> + if (!ipv6_mod_enabled())
> break;
>
> rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0,
>
seems to me the existing check is more appropriate and relevant for the
code path: is ipv6 enabled on this device versus is ipv6 enabled at all.
^ permalink raw reply
* Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Ahern @ 2019-02-25 16:57 UTC (permalink / raw)
To: Eric Dumazet, Hangbin Liu; +Cc: netdev, Stefano Brivio, David S . Miller
In-Reply-To: <1dd57d9d-fed2-67b8-ac28-7ef3681eeed2@gmail.com>
On 2/25/19 9:39 AM, Eric Dumazet wrote:
>
>
> On 02/25/2019 12:17 AM, Hangbin Liu wrote:
>> On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote:
>>>
>>>
>>> On 02/24/2019 08:12 PM, Hangbin Liu wrote:
>>>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
>>>> at running time.
>>>>
>>>
>>>
>>> Why is it better exactly ?
>>>
>>> IPv6 can be enabled on the host, but disabled per device
>>>
>>> /proc/sys/net/ipv6/conf/{name}/disable_ipv6
>>
>> Sorry, it looks I didn't make it clear in the commit description.
>> This issue only occurs when IPv6 is disabled at boot time as there is
>> no IPv6 route entry. Disable ipv6 on specific interface is not affected.
>> So check ipv6_mod_enabled() is enough and we don't need to worry about
>> the rcu_read_lock or the dev status.
>>
>> Should I update the commit description?
>
> Certainly. Are you telling us skb->dev could be NULL here ?
>
> Because rcu_read_lock() should already be asserted.
>
Same response as geneve. The existing check is more appropriate and
relevant for the code path: is ipv6 enabled on this device versus is
ipv6 enabled at all.
^ permalink raw reply
* Re: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed
From: Florian Fainelli @ 2019-02-25 17:21 UTC (permalink / raw)
To: Parshuram Raju Thombare, nicolas.ferre@microchip.com,
davem@davemloft.net, netdev@vger.kernel.org, andrew@lunn.ch,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Piotr Sroka, Jan Kotas
In-Reply-To: <CO2PR07MB2469FCDFEE81BE4D3ECE9C92C17A0@CO2PR07MB2469.namprd07.prod.outlook.com>
On 2/25/19 1:11 AM, Parshuram Raju Thombare wrote:
>> Le 2/22/19 à 12:12 PM, Parshuram Thombare a écrit :
>>> This patch add support for PCS (for SGMII interface) and 2.5Gbps MAC
>>> in Cadence ethernet controller driver.
>>
>> At a high level you don't seem to be making use of PHYLINK so which 2.5Gbps
>> interfaces do you actually support?
>>
>
> New ethernet controller have MAC which support 2.5G speed.
> Also there is addition of PCS and SGMII interface.
I should have asked this more clearly: have you tested with SFP modules
for instance? If you want to be able to reliably support 2500baseT
and/or 2500baseX with hot plugging of such modules, you need to
implement PHYLINK for that network driver, there is no other way around.
>
>>>
>>> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
>>> ---
>>
>> [snip]
>>
>>> @@ -361,26 +361,50 @@ static int macb_mdio_write(struct mii_bus *bus, int
>> mii_id, int regnum,
>>> * macb_set_tx_clk() - Set a clock to a new frequency
>>> * @clk Pointer to the clock to change
>>> * @rate New frequency in Hz
>>> + * @interafce Phy interface
>>
>> Typo: @interface and this is an unrelated change.
>>
>>> * @dev Pointer to the struct net_device
>>> */
>>> -static void macb_set_tx_clk(struct clk *clk, int speed, struct
>>> net_device *dev)
>>> +static void macb_set_tx_clk(struct clk *clk, int speed,
>>> + phy_interface_t interface, struct net_device *dev)
>>> {
>>> long ferr, rate, rate_rounded;
>>>
>>> if (!clk)
>>> return;
>>>
>>> - switch (speed) {
>>> - case SPEED_10:
>>> + if (interface == PHY_INTERFACE_MODE_GMII ||
>>> + interface == PHY_INTERFACE_MODE_MII) {
>>> + switch (speed) {
>>> + case SPEED_10:> rate = 2500000;
>>
>> You need to add one tab to align rate and break.
>
> Do you mean a tab each for rate and break lines ?
> All switch statements are aligned at a tab. I am not sure how does case and rate got on same line.
It should look like this:
switch (cond) {
case cond1:
do_something();
break;
etc.
>
>>
>>> break;
>>> - case SPEED_100:
>>> + case SPEED_100:
>>> rate = 25000000;
>>> break;
>>> - case SPEED_1000:
>>> + case SPEED_1000:
>>> rate = 125000000;
>>> break;
>>> - default:
>>> + default:
>>> + return;
>>> + }
>>> + } else if (interface == PHY_INTERFACE_MODE_SGMII) {
>>> + switch (speed) {
>>> + case SPEED_10:
>>> + rate = 1250000;
>>> + break;
>>> + case SPEED_100:
>>> + rate = 12500000;
>>> + break;
>>> + case SPEED_1000:
>>> + rate = 125000000;
>>> + break;
>>> + case SPEED_2500:
>>> + rate = 312500000;
>>> + break;
>>> + default:
>>> + return;
>>
>> The indentation is broken here and you can greatly simplify this with a simple
>> function that returns speed * 1250 and does an initial check for unsupported
>> speeds.
>>
>
> I ran checkpatch.pl and all indentation issues were cleared. But I think having function
> is better option, I will make that change.
>
>>> + }
>>> + } else {
>>> return;
>>> }
>>>
>>> @@ -410,30 +434,49 @@ static void macb_handle_link_change(struct
>>> net_device *dev)
>>>
>>> spin_lock_irqsave(&bp->lock, flags);
>>>
>>> - if (phydev->link) {
>>> - if ((bp->speed != phydev->speed) ||
>>> - (bp->duplex != phydev->duplex)) {
>>> - u32 reg;
>>> -
>>> - reg = macb_readl(bp, NCFGR);
>>> - reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>>> - if (macb_is_gem(bp))
>>> - reg &= ~GEM_BIT(GBE);
>>> + if (phydev->link && (bp->speed != phydev->speed ||
>>> + bp->duplex != phydev->duplex)) {
>>> + u32 reg;
>>>
>>> - if (phydev->duplex)
>>> - reg |= MACB_BIT(FD);
>>> + reg = macb_readl(bp, NCFGR);
>>> + reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>>> + if (macb_is_gem(bp))
>>> + reg &= ~GEM_BIT(GBE);
>>> + if (phydev->duplex)
>>> + reg |= MACB_BIT(FD);
>>> + macb_or_gem_writel(bp, NCFGR, reg);
>>> +
>>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII &&
>>> + (phydev->speed == SPEED_1000 ||
>>> + phydev->speed == SPEED_2500)) {
>>> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED) {
>>> + reg = gem_readl(bp, NCR) &
>>> + ~GEM_BIT(TWO_PT_FIVE_GIG);
>>> + gem_writel(bp, NCR, reg);
>>> + }
>>
>> If you are making correct use of the capabilities then there is no point in re-
>> checking them here. If you allowed the MAC to advertise 2.5Gbps then it is de-
>> facto SGMII capable.
>
> PHY_INTERFACE_MODE_SGMII is selected only on the basis of presence of PCS.
> This additional check is to make sure PHY also support 1G/2.5G.
My point is that you should never get to that function with
bp->phy_interface == PHY_INTERFACE_MODE_SGMII if you have done necessary
verifications at the time you connect to the PHY. If you let
PHY_INTERFACE_MODE_SGMII go through on a Cadence IP version that does
not have the MACB_CAPS_TWO_PT_FIVE_GIG_SPEED capability bit set, then
this is broken.
>
>>> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
>>> + gem_readl(bp, NCFGR));
>>> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED
>> &&
>>> + phydev->speed == SPEED_2500)
>>> + gem_writel(bp, NCR, gem_readl(bp, NCR) |
>>> + GEM_BIT(TWO_PT_FIVE_GIG));
>>> + } else if (phydev->speed == SPEED_1000) {
>>> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
>>> + gem_readl(bp, NCFGR));
>>> + } else {
>>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> {
>>> + reg = gem_readl(bp, NCFGR);
>>> + reg &= ~(GEM_BIT(SGMIIEN) |
>> GEM_BIT(PCSSEL));
>>> + gem_writel(bp, NCFGR, reg);
>>> + }
>>> if (phydev->speed == SPEED_100)
>>> - reg |= MACB_BIT(SPD);
>>> - if (phydev->speed == SPEED_1000 &&
>>> - bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>>> - reg |= GEM_BIT(GBE);
>>> -
>>> - macb_or_gem_writel(bp, NCFGR, reg);
>>> -
>>> - bp->speed = phydev->speed;
>>> - bp->duplex = phydev->duplex;
>>> - status_change = 1;
>>> + macb_writel(bp, NCFGR, MACB_BIT(SPD) |
>>> + macb_readl(bp, NCFGR));
>>> }
>>
>> There is a lot of repetition while setting the GBE bit which always set based on
>> speed == 1000 irrespective of the mode, so take that part out of the if () else if ()
>> else () clauses.
>>
>
> Ok, I will change it.
>
>>> +
>>> + bp->speed = phydev->speed;
>>> + bp->duplex = phydev->duplex;
>>> + status_change = 1;
>>> }
>>>
>>> if (phydev->link != bp->link) {
>>> @@ -453,7 +496,8 @@ static void macb_handle_link_change(struct net_device
>> *dev)
>>> /* Update the TX clock rate if and only if the link is
>>> * up and there has been a link change.
>>> */
>>> - macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
>>> + macb_set_tx_clk(bp->tx_clk, phydev->speed,
>>> + bp->phy_interface, dev);
>>>
>>> netif_carrier_on(dev);
>>> netdev_info(dev, "link up (%d/%s)\n", @@ -543,10
>> +587,16 @@ static
>>> int macb_mii_probe(struct net_device *dev)
>>> }
>>>
>>> /* mask with MAC supported features */
>>> - if (macb_is_gem(bp) && bp->caps &
>> MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>>> - phy_set_max_speed(phydev, SPEED_1000);
>>> - else
>>> - phy_set_max_speed(phydev, SPEED_100);
>>> + if (macb_is_gem(bp)) {
>>
>> You have changed the previous logic that also checked for
>> MACB_CAPS_GIGABIT_MODE_AVAILABLE, why?
>
> My understanding is all GEM (ID >= 0x2) support GIGABIT mode.
> Was there any other reason for this check ?
Well, if anyone would know, it would be you, I don't work for Cadence
nor have access to the internal IP documentation.
>
>>> + linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
>>> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
>>> +
>> linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>> + phydev->supported);
>>> + } else {
>>> + linkmode_copy(phydev->supported, PHY_BASIC_FEATURES);
>>> + }
>>> +
>>> + linkmode_copy(phydev->advertising, phydev->supported);
>>>
>>> if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
>>> phy_remove_link_mode(phydev,
>>> @@ -2217,8 +2267,6 @@ static void macb_init_hw(struct macb *bp)
>>> macb_set_hwaddr(bp);
>>>
>>> config = macb_mdc_clk_div(bp);
>>> - if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>>> - config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>>> config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data
>> aligned */
>>> config |= MACB_BIT(PAE); /* PAuse Enable */
>>> config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
>>> @@ -3255,6 +3303,23 @@ static void macb_configure_caps(struct macb *bp,
>>> dcfg = gem_readl(bp, DCFG1);
>>> if (GEM_BFEXT(IRQCOR, dcfg) == 0)
>>> bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
>>> + if (GEM_BFEXT(NO_PCS, dcfg) == 0)
>>> + bp->caps |= MACB_CAPS_PCS;
>>> + switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
>>> + case MACB_GEM7016_IDNUM:
>>> + case MACB_GEM7017_IDNUM:
>>> + case MACB_GEM7017A_IDNUM:
>>> + case MACB_GEM7020_IDNUM:
>>> + case MACB_GEM7021_IDNUM:
>>> + case MACB_GEM7021A_IDNUM:
>>> + case MACB_GEM7022_IDNUM:
>>> + if (bp->caps & MACB_CAPS_PCS)
>>> + bp->caps |= MACB_CAPS_TWO_PT_FIVE_GIG_SPEED;
>>> + break;
>>> +
>>> + default:
>>> + break;
>>> + }
>>> dcfg = gem_readl(bp, DCFG2);
>>> if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF)))
>> == 0)
>>> bp->caps |= MACB_CAPS_FIFO_MODE;
>>> @@ -4110,7 +4175,28 @@ static int macb_probe(struct platform_device
>> *pdev)
>>> else
>>> bp->phy_interface = PHY_INTERFACE_MODE_MII;
>>> } else {
>>> + switch (err) {
>>> + case PHY_INTERFACE_MODE_SGMII:
>>> + if (bp->caps & MACB_CAPS_PCS) {
>>> + bp->phy_interface = PHY_INTERFACE_MODE_SGMII;
>>> + break;
>>> + }
>>
>> If SGMII was selected on a version of the IP that does not support it, then falling
>> back to GMII or MII does not sound correct, this is a hard error that must be
>> handled as such.
>> --
>> Florian
>
> My intention was to continue (instead of failing) with whatever functionality is available.
> Can we have some error message and continue with what is available ?
This is probably not going to help anyone, imagine you incorrectly
specified a 'phy-mode' property in the Device Tree and you have to dig
into the driver in the function that sets the clock rate to find out
what you just defaulted to 1Gbits/GMII for instance. This is not user
friendly at all and will increase the support burden on your end as
well, if SGMII is specified and the IP does not support it, detect it
and return an error, how that propagates, either as a probe failure or
the inability to open the network device, your call.
--
Florian
^ permalink raw reply
* Re: [PATCH] net: dsa: read mac address from DT for slave device
From: Florian Fainelli @ 2019-02-25 17:27 UTC (permalink / raw)
To: xiaofeis
Cc: Vinod Koul, David S. Miller, linux-arm-msm, Bjorn Andersson,
Andrew Lunn, Vivien Didelot, Niklas Cassel, netdev
In-Reply-To: <fc02f99d3a84ace86f6c2cb6411b4b33@codeaurora.org>
On 2/25/19 5:28 AM, xiaofeis@codeaurora.org wrote:
> Hi Florian
>
> We have two slave DSA interfaces, wan0 and lan0, one is for wan port,
> and the other is for lan port. Customer has it's mac address pool, they
> want
> to assign the mac address from the pool on wan0, lan0, and other
> interfaces like
> wifi, bt. Coreboot/uboot will populate it to the DTS node, so the driver
> can
> get it from it's node. For DSA slave interface, it already has it's own
> DTS node, it's
> easy to just add one porperty "local-mac-address" there for the usage in
> DSA driver.
>
> If not use DSA framework, normally we will use eth0.x and eth0.y for wan
> and lan.
> On this case, customer usually also assign the MAC address on these
> logical interface
> from it's pool.
OK, but this is not necessary per my previous explanation: the CPU <=>
WAN pipe is a separate broadcast domain (otherwise it is a security hole
since you exposing LAN machines to the outside world), and so there is
no need for a separate MAC address. It might be convenient to have one,
especially for the provider, if they run a management software (e.g.:
TR69), but it is not required per-se.
Let me ask a secondary question here, how many Ethernet MACs connect to
the switch in this configuration? Is there one that is supposed to be
assigned all LAN traffic and one that is supposed to be assigned all WAN
traffic? If so, then what you are doing makes even less
>
> On 2019-02-22 23:43, Florian Fainelli wrote:
>> On 2/22/19 4:58 AM, Vinod Koul wrote:
>>> From: Xiaofei Shen <xiaofeis@codeaurora.org>
>>>
>>> Before creating a slave netdevice, get the mac address from DTS and
>>> apply in case it is valid.
>>
>> Can you explain your use case in details?
>>
>> Assigning a MAC address to a network device that represents a switch
>> port does not quite make sense in general. The switch port is really
>> representing one end of a pipe, so one side you have stations and on the
>> other side, you have the CPU/management Ethernet MAC controller's MAC
>> address which constitutes a station as well. The DSA slave network
>> devices are just software constructs meant to steer traffic towards
>> specific ports of the switch, but they are all from the perpsective of
>> traffic reaching the CPU Port in the first place, therefore traffic that
>> is generally a known unicast Ethernet frame with the CPU's MAC address
>> as MAC DA (and of course all types of unknown MC, management traffic
>> etc.)
>>
>> By default, DSA switch need to come up in a configuration where all
>> ports (except CPU/management) must be strictly separate from every other
>> port such that we can achieve what a standalone Ethernet NIC would do.
>> This works because all ports are isolated from one another, so there is
>> no cross talk and so having the same MAC address (the one from the CPU)
>> on the DSA slave network devices just works, each port is a separate
>> broadcast domain.
>>
>> Once you start bridging one or ore ports, the bridge root port will have
>> a MAC address, most likely the one the CPU/management Ethernet MAC, but
>> similarly, this is not an issue and that's exactly how a software bridge
>> would work as well.
>>
>>>
>>> Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org>
>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>> ---
>>> include/net/dsa.h | 1 +
>>> net/dsa/dsa2.c | 1 +
>>> net/dsa/slave.c | 5 ++++-
>>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>>> index b3eefe8e18fd..aa24ce756679 100644
>>> --- a/include/net/dsa.h
>>> +++ b/include/net/dsa.h
>>> @@ -198,6 +198,7 @@ struct dsa_port {
>>> unsigned int index;
>>> const char *name;
>>> const struct dsa_port *cpu_dp;
>>> + const char *mac;
>>> struct device_node *dn;
>>> unsigned int ageing_time;
>>> u8 stp_state;
>>> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>>> index a1917025e155..afb7d9fa42f6 100644
>>> --- a/net/dsa/dsa2.c
>>> +++ b/net/dsa/dsa2.c
>>> @@ -261,6 +261,7 @@ static int dsa_port_setup(struct dsa_port *dp)
>>> int err = 0;
>>>
>>> memset(&dp->devlink_port, 0, sizeof(dp->devlink_port));
>>> + dp->mac = of_get_mac_address(dp->dn);
>>>
>>> if (dp->type != DSA_PORT_TYPE_UNUSED)
>>> err = devlink_port_register(ds->devlink, &dp->devlink_port,
>>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>>> index a3fcc1d01615..8e64c4e947c6 100644
>>> --- a/net/dsa/slave.c
>>> +++ b/net/dsa/slave.c
>>> @@ -1308,7 +1308,10 @@ int dsa_slave_create(struct dsa_port *port)
>>> slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>>> slave_dev->hw_features |= NETIF_F_HW_TC;
>>> slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>>> - eth_hw_addr_inherit(slave_dev, master);
>>> + if (port->mac && is_valid_ether_addr(port->mac))
>>> + ether_addr_copy(slave_dev->dev_addr, port->mac);
>>> + else
>>> + eth_hw_addr_inherit(slave_dev, master);
>>> slave_dev->priv_flags |= IFF_NO_QUEUE;
>>> slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>>> slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
>>>
--
Florian
^ permalink raw reply
* Re: [PATCH v2] net: dsa: fix a leaked reference by adding missing of_node_put
From: David Miller @ 2019-02-25 17:35 UTC (permalink / raw)
To: wen.yang99
Cc: andrew, vivien.didelot, f.fainelli, netdev, linux-kernel,
wang.yi59
In-Reply-To: <1551079339-30270-1-git-send-email-wen.yang99@zte.com.cn>
From: Wen Yang <wen.yang99@zte.com.cn>
Date: Mon, 25 Feb 2019 15:22:19 +0800
> The call to of_parse_phandle returns a node pointer with refcount
> incremented thus it must be explicitly decremented after the last
> usage.
>
> Detected by coccinelle with the following warnings:
> ./net/dsa/port.c:294:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 284, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:627:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:630:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:636:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:639:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
> Reviewed-by: "David S. Miller" <davem@davemloft.net>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] tcp: remove unused parameter of tcp_sacktag_bsearch()
From: David Miller @ 2019-02-25 17:38 UTC (permalink / raw)
To: ap420073; +Cc: edumazet, netdev
In-Reply-To: <20190225094233.16231-1-ap420073@gmail.com>
From: Taehee Yoo <ap420073@gmail.com>
Date: Mon, 25 Feb 2019 18:42:33 +0900
> parameter state in the tcp_sacktag_bsearch() is not used.
> So, it can be removed.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Applied, thanks Taehee.
^ permalink raw reply
* Re: [PATCH net-next v2 5/7] net: sched: pie: add more cases to auto-tune alpha and beta
From: David Miller @ 2019-02-25 17:40 UTC (permalink / raw)
To: lesliemonis
Cc: jhs, netdev, dave, tahiliani, dhavaljkhandla26, hrishihiraskar,
bmanish15597, sdp.sachin
In-Reply-To: <20190225102051.12268-6-lesliemonis@gmail.com>
From: Leslie Monis <lesliemonis@gmail.com>
Date: Mon, 25 Feb 2019 15:50:49 +0530
> + power = 100;
> + while (q->vars.prob < MAX_PROB / (u64)power &&
You have to use div_u64() or similar when doing 64-bit divisons otherwise
you'll get build failures on 32-bit platforms.
^ permalink raw reply
* Re: [PATCH] tcp: clean up SOCK_DEBUG()
From: David Miller @ 2019-02-25 17:41 UTC (permalink / raw)
To: laoar.shao; +Cc: netdev, shaoyafang
In-Reply-To: <1551090828-31988-1-git-send-email-laoar.shao@gmail.com>
From: Yafang Shao <laoar.shao@gmail.com>
Date: Mon, 25 Feb 2019 18:33:48 +0800
> Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
> TCP are not needed now.
> We'd better clean up it.
>
> [1] https://patchwork.ozlabs.org/patch/1035573/
> [2] https://patchwork.ozlabs.org/patch/1040533/
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net v3 0/2] selftests: pmtu: fix and increase coverage
From: Stefano Brivio @ 2019-02-25 17:46 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, David S. Miller, David Ahern, Sabrina Dubroca
In-Reply-To: <cover.1551106591.git.pabeni@redhat.com>
On Mon, 25 Feb 2019 16:08:35 +0100
Paolo Abeni <pabeni@redhat.com> wrote:
> This series includes a fixup for the pmtu.sh test script, related to IPv6
> address management, and adds coverage for the recently reported and fixed
> PMTU exception issue
>
> v2 -> v3:
> - more cleanups
>
> v1 -> v2:
> - several script cleanups
>
> Paolo Abeni (2):
> selftests: pmtu: disable DAD in all namespaces
> selftests: pmtu: add explicit tests for PMTU exceptions cleanup
Thanks for following up!
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
--
Stefano
^ permalink raw reply
* Re: [PATCH net-next resend 1/2] net: dsa: mv88e6xxx: Default CMODE to 1000BaseX only on 6390X
From: David Miller @ 2019-02-25 17:47 UTC (permalink / raw)
To: marek.behun; +Cc: netdev, andrew, f.fainelli
In-Reply-To: <20190225113955.25700-2-marek.behun@nic.cz>
From: Marek Behún <marek.behun@nic.cz>
Date: Mon, 25 Feb 2019 12:39:54 +0100
> Commit 787799a9d555 sets the SERDES interfaces of 6390 and 6390X to
> 1000BaseX, but this is only needed on 6390X, since there are SERDES
> interfaces which can be used on lower ports on 6390.
>
> This commit fixes this by returning to previous behaviour on 6390.
> (Previous behaviour means that CMODE is not set at all if requested mode
> is NA).
>
> This is needed on Turris MOX, where the 88e6190 is connected to CPU in
> 2500BaseX mode.
>
> Fixes: 787799a9d555 ("net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX")
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next resend 2/2] net: dsa: mv88e6xxx: Fix phylink_validate for Topaz family
From: David Miller @ 2019-02-25 17:47 UTC (permalink / raw)
To: marek.behun; +Cc: netdev, andrew, f.fainelli
In-Reply-To: <20190225113955.25700-3-marek.behun@nic.cz>
From: Marek Behún <marek.behun@nic.cz>
Date: Mon, 25 Feb 2019 12:39:55 +0100
> The Topaz family should have different phylink_validate method from the
> Peridot, since on Topaz the port supporting 2500BaseX mode is port 5,
> not 9 and 10.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next v2 5/7] net: sched: pie: add more cases to auto-tune alpha and beta
From: Leslie Monis @ 2019-02-25 18:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20190225.094038.1156384775297802527.davem@davemloft.net>
On Mon, Feb 25, 2019 at 09:40:38AM -0800, David Miller wrote:
> From: Leslie Monis <lesliemonis@gmail.com>
> Date: Mon, 25 Feb 2019 15:50:49 +0530
>
> > + power = 100;
> > + while (q->vars.prob < MAX_PROB / (u64)power &&
>
> You have to use div_u64() or similar when doing 64-bit divisons otherwise
> you'll get build failures on 32-bit platforms.
Alright. Thanks for the feedback.
^ permalink raw reply
* Re: [PATCH net] net: sched: act_tunnel_key: fix NULL pointer dereference during init
From: David Miller @ 2019-02-25 18:16 UTC (permalink / raw)
To: vladbu; +Cc: netdev, dcaratti, jhs, xiyou.wangcong, jiri, wenxu, roid
In-Reply-To: <20190225152827.8741-1-vladbu@mellanox.com>
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:28:27 +0200
> Metadata pointer is only initialized for action TCA_TUNNEL_KEY_ACT_SET, but
> it is unconditionally dereferenced in tunnel_key_init() error handler.
> Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
>
> Fixes: ee28bb56ac5b ("net/sched: fix memory leak in act_tunnel_key_init()")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: David Miller @ 2019-02-25 18:18 UTC (permalink / raw)
To: vladbu; +Cc: netdev, jhs, xiyou.wangcong, jiri
In-Reply-To: <20190225153831.10037-1-vladbu@mellanox.com>
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:38:31 +0200
> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.
>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: David Miller @ 2019-02-25 18:25 UTC (permalink / raw)
To: vladbu; +Cc: netdev, jhs, xiyou.wangcong, jiri
In-Reply-To: <20190225154544.10453-1-vladbu@mellanox.com>
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:45:44 +0200
> Function tc_dump_chain() obtains and releases block->lock on each iteration
> of its inner loop that dumps all chains on block. Outputting chain template
> info is fast operation so locking/unlocking mutex multiple times is an
> overhead when lock is highly contested. Modify tc_dump_chain() to only
> obtain block->lock once and dump all chains without releasing it.
>
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks Vlad.
^ permalink raw reply
* Re: [PATCH] tun: fix blocking read
From: David Miller @ 2019-02-25 18:25 UTC (permalink / raw)
To: eric.dumazet; +Cc: mail, netdev, linux-kernel
In-Reply-To: <b727b3bb-ec46-12bf-dd38-0363c9ca5854@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 25 Feb 2019 07:53:08 -0800
>
>
> On 02/24/2019 10:12 PM, David Miller wrote:
>> From: Timur Celik <mail@timurcelik.de>
>> Date: Sat, 23 Feb 2019 12:53:13 +0100
>>
>>> This patch moves setting of the current state into the loop. Otherwise
>>> the task may end up in a busy wait loop if none of the break conditions
>>> are met.
>>>
>>> Signed-off-by: Timur Celik <mail@timurcelik.de>
>>
>> Applied and queued up for -stable, thanks.
>>
>
> First part of the patch was matching the changelog, however
> the second change was not really needed, or should have used
>
> __set_current_state(TASK_RUNNING);
Timur, please followup.
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH net-next 3/4] e1000e: Fix -Wformat-truncation warnings
From: Neftin, Sasha @ 2019-02-25 18:26 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: open list, moderated list:INTEL ETHERNET DRIVERS, davem
In-Reply-To: <20190222040929.16657-4-f.fainelli@gmail.com>
On 22/02/2019 6:09, Florian Fainelli wrote:
> Provide precision hints to snprintf() since we know the destination
> buffer size of the RX/TX ring names are IFNAMSIZ + 5 - 1. This fixes the
> following warnings:
>
> drivers/net/ethernet/intel/e1000e/netdev.c: In function
> 'e1000_request_msix':
> drivers/net/ethernet/intel/e1000e/netdev.c:2109:13: warning: 'snprintf'
> output may be truncated before the last format character
> [-Wformat-truncation=]
> "%s-rx-0", netdev->name);
> ^
> drivers/net/ethernet/intel/e1000e/netdev.c:2107:3: note: 'snprintf'
> output between 6 and 21 bytes into a destination of size 20
> snprintf(adapter->rx_ring->name,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sizeof(adapter->rx_ring->name) - 1,
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> "%s-rx-0", netdev->name);
> ~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/intel/e1000e/netdev.c:2125:13: warning: 'snprintf'
> output may be truncated before the last format character
> [-Wformat-truncation=]
> "%s-tx-0", netdev->name);
> ^
> drivers/net/ethernet/intel/e1000e/netdev.c:2123:3: note: 'snprintf'
> output between 6 and 21 bytes into a destination of size 20
> snprintf(adapter->tx_ring->name,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sizeof(adapter->tx_ring->name) - 1,
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> "%s-tx-0", netdev->name);
> ~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 736fa51878f8..7acc61e4f645 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -2106,7 +2106,7 @@ static int e1000_request_msix(struct e1000_adapter *adapter)
> if (strlen(netdev->name) < (IFNAMSIZ - 5))
> snprintf(adapter->rx_ring->name,
> sizeof(adapter->rx_ring->name) - 1,
> - "%s-rx-0", netdev->name);
> + "%.14s-rx-0", netdev->name);
> else
> memcpy(adapter->rx_ring->name, netdev->name, IFNAMSIZ);
> err = request_irq(adapter->msix_entries[vector].vector,
> @@ -2122,7 +2122,7 @@ static int e1000_request_msix(struct e1000_adapter *adapter)
> if (strlen(netdev->name) < (IFNAMSIZ - 5))
> snprintf(adapter->tx_ring->name,
> sizeof(adapter->tx_ring->name) - 1,
> - "%s-tx-0", netdev->name);
> + "%.14s-tx-0", netdev->name);
> else
> memcpy(adapter->tx_ring->name, netdev->name, IFNAMSIZ);
> err = request_irq(adapter->msix_entries[vector].vector,
>
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Lorenz Bauer @ 2019-02-25 18:26 UTC (permalink / raw)
To: Martin Lau
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20190223004431.osqrn3u3lp7a6zww@kafai-mbp.dhcp.thefacebook.com>
On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
>
> On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > to a known connection. However, there is one corner case: no sockets are
> > created if SYN cookies are active. This means that the final ACK in the
> > 3WHS is misclassified.
> >
> > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > and then check whether a packet is a valid SYN cookie ACK.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> > include/uapi/linux/bpf.h | 18 ++++++++++-
> > net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index bcdd2474eee7..bc2af87e9621 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2359,6 +2359,21 @@ union bpf_attr {
> > * Return
> > * A **struct bpf_tcp_sock** pointer on success, or NULL in
> > * case of failure.
> > + *
> > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > + * Description
> > + * Check whether iph and th contain a valid SYN cookie ACK for
> > + * the listening socket in sk.
> > + *
> > + * iph points to the start of the IPv4 or IPv6 header, while
> > + * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > + *
> > + * th points to the start of the TCP header, while th_len contains
> > + * sizeof(struct tcphdr).
> > + *
> > + * Return
> > + * 0 if iph and th are a valid SYN cookie ACK, or a negative error
> > + * otherwise.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2457,7 +2472,8 @@ union bpf_attr {
> > FN(spin_lock), \
> > FN(spin_unlock), \
> > FN(sk_fullsock), \
> > - FN(tcp_sock),
> > + FN(tcp_sock), \
> > + FN(sk_check_syncookie),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 85749f6ec789..9e68897cc7ed 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> > .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> > };
> >
> > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
>
> > + struct tcphdr *, th, u32, th_len)
> > +{
> > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
>
> > + u32 cookie;
> > + int ret;
> > +
> > + if (unlikely(th_len < sizeof(*th)))
> > + return -EINVAL;
> > +
> > + /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > + if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> From the test program in patch 3, the "sk" here is obtained from
> bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> even if there is a request_sock. Does it make sense to check
> syncookie if there is already a request_sock?
No, that doesn't make a lot of sense. I hadn't realised that
sk_lookup_tcp only returns full sockets.
This means we need a way to detect that there is a request sock for a
given tuple.
* adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
* drop the sk argument and do the necessary lookups in the helper
itself, but that also
wastes a call to __inet_lookup_listener
* skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)
For context: ultimately we want use this to answer the question: does
this (encapsulated)
packet contain a payload destined to a local socket? Amongst the edge
cases we need to
handle are ICMP Packet Too Big messages and SYN cookies. A solution
would be to hide
all this in an "uber" helper that takes pointers to the L3 / L4
headers and returns a verdict,
but that seems a bit gross.
>
> > + return -EINVAL;
> > +
> > + if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> Should tcp_synq_no_recent_overflow(tp) be checked also?
>
Yes, not sure how that slipped out.
> > + return -EINVAL;
> > +
> > + if (!th->ack || th->rst)
> How about th->syn?
>
Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.
> > + return -ENOENT;
> > +
> > + cookie = ntohl(th->ack_seq) - 1;
> > +
> > + switch (sk->sk_family) {
> > + case AF_INET:
> > + if (unlikely(iph_len < sizeof(struct iphdr)))
> > + return -EINVAL;
> > +
> > + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > + break;
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + case AF_INET6:
> > + if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > + return -EINVAL;
> > +
> > + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > + break;
> > +#endif /* CONFIG_IPV6 */
> > +
> > + default:
> > + return -EPROTONOSUPPORT;
> > + }
> > +
> > + if (ret > 0)
> > + return 0;
> > +
> > + return -ENOENT;
> > +#else
> > + return -ENOTSUP;
> > +#endif
> > +}
> > +
> > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > + .func = bpf_sk_check_syncookie,
> > + .gpl_only = true,
> > + .pkt_access = true,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_SOCKET,
> I think it should be ARG_PTR_TO_TCP_SOCK
>
> > + .arg2_type = ARG_PTR_TO_MEM,
> > + .arg3_type = ARG_CONST_SIZE,
> > + .arg4_type = ARG_PTR_TO_MEM,
> > + .arg5_type = ARG_CONST_SIZE,
> > +};
> > +
> > #endif /* CONFIG_INET */
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH net] geneve: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Miller @ 2019-02-25 18:28 UTC (permalink / raw)
To: dsa; +Cc: liuhangbin, netdev, sbrivio
In-Reply-To: <00742c9b-9cf8-1025-2c3a-0684cd8f6c7f@cumulusnetworks.com>
From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 25 Feb 2019 09:57:08 -0700
> On 2/24/19 9:13 PM, Hangbin Liu wrote:
>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
>> at running time.
>>
>> Fixes: c0a47e44c098 ("geneve: should not call rt6_lookup() when ipv6 was disabled")
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> ---
>> drivers/net/geneve.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 3377ac66a347..42c5e43ece68 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -1514,7 +1514,7 @@ static void geneve_link_config(struct net_device *dev,
>> case AF_INET6: {
>> struct rt6_info *rt;
>>
>> - if (!__in6_dev_get(dev))
>> + if (!ipv6_mod_enabled())
>> break;
>>
>> rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0,
>>
>
> seems to me the existing check is more appropriate and relevant for the
> code path: is ipv6 enabled on this device versus is ipv6 enabled at all.
Agreed.
^ 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