* Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: Hannes Frederic Sowa @ 2013-09-07 22:03 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Eldad Zack, netdev, davem, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <20130907213445.GB1442@minipsycho.orion>
Hello!
On Sat, Sep 07, 2013 at 11:34:45PM +0200, Jiri Pirko wrote:
> Sat, Sep 07, 2013 at 06:46:12PM CEST, eldad@fogrefinery.com wrote:
> >
> >
> >On Sat, 7 Sep 2013, Jiri Pirko wrote:
> >
> >> Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
> >> >
> >> >Hi Jiri,
> >> >
> >> >On Fri, 6 Sep 2013, Jiri Pirko wrote:
> >> >
> >> >> In rfc4942 and rfc2460 I cannot find anything which would implicate to
> >> >> drop packets which have only padding in tlv.
> >> >
> >> >NAK from my side.
> >> >Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
> >>
> >> I did.
> >>
> >> >
> >> >While it doesn't specifically discusses this corner case, you can
> >> >understand from "There is no legitimate reason for padding beyond the
> >> >next eight octet..." that there's also no legitimate reason for an
> >> >option header containing only padding.
There could be a legitimate reason: if a firewall wants to discard a HBH
or DOH it could easily zero out the option space instead of rearranging
the segment. But to make this happen in the general case all limits
regarding padding options would have to be dropped, too. That said,
I don't see this as an valid argument.
> >>
> >> Okay. I'm glad you agree with me and that we both understand the rfc the
> >> same way. And since the rfc does not say that "here's no legitimate
> >> reason for an option header containing only padding", this should be
> >> possible. I say we respect rfc and do not add stronger restrictions than
> >> it dictates. No need for them.
> >
> >Strictly speaking, this RFC is informational, so it is doesn't dictate
> >per se. I hope you don't suggest removing the other checks as well...
>
> If there are some that just plainly assumes something and does
> restrictions without any solid argument, yes remove them.
>
> >
> >> >I can't imagine a sane use-case for this.
> >>
> >> rfcs are not about sanity...
> >
> >Great, we're in agreement again :) But I think the networking code is
> >or should be.
> >What IPv6 stack would generate such a packet, given that the only usage
> >of the padding options is to align other options?
>
> But why not? I do not see anything evil about that. And rfc allows it.
It is easier to generate valid ipv6 frames which are fragmented in the
header chain. This makes it harder for firewalls to match packets etc.
This was a known attack vector with e.g. router advertisments and
fragmentation. Filters implemented in switches could be circumvented
because they did not match the fragmented RA but the host correctly did
reassemble the packet.
> >Why should I accept a packet which is most likely an artificially
> >crafted packet (RFC 3514)?
Of course, dropping this check won't do big harm. But I actually like
the strictness of the ipv6 header chain checks and dropping this just
for the IPv6 Ready Logo doesn't seem right. Perhaps one could talk to
the people of tahi.org first?
Greetings,
Hannes
^ permalink raw reply
* Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: Jiri Pirko @ 2013-09-07 21:34 UTC (permalink / raw)
To: Eldad Zack; +Cc: netdev, davem, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <alpine.LNX.2.00.1309071819310.24145@heraclitus>
Sat, Sep 07, 2013 at 06:46:12PM CEST, eldad@fogrefinery.com wrote:
>
>
>On Sat, 7 Sep 2013, Jiri Pirko wrote:
>
>> Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
>> >
>> >Hi Jiri,
>> >
>> >On Fri, 6 Sep 2013, Jiri Pirko wrote:
>> >
>> >> In rfc4942 and rfc2460 I cannot find anything which would implicate to
>> >> drop packets which have only padding in tlv.
>> >
>> >NAK from my side.
>> >Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
>>
>> I did.
>>
>> >
>> >While it doesn't specifically discusses this corner case, you can
>> >understand from "There is no legitimate reason for padding beyond the
>> >next eight octet..." that there's also no legitimate reason for an
>> >option header containing only padding.
>>
>> Okay. I'm glad you agree with me and that we both understand the rfc the
>> same way. And since the rfc does not say that "here's no legitimate
>> reason for an option header containing only padding", this should be
>> possible. I say we respect rfc and do not add stronger restrictions than
>> it dictates. No need for them.
>
>Strictly speaking, this RFC is informational, so it is doesn't dictate
>per se. I hope you don't suggest removing the other checks as well...
If there are some that just plainly assumes something and does
restrictions without any solid argument, yes remove them.
>
>> >I can't imagine a sane use-case for this.
>>
>> rfcs are not about sanity...
>
>Great, we're in agreement again :) But I think the networking code is
>or should be.
>What IPv6 stack would generate such a packet, given that the only usage
>of the padding options is to align other options?
But why not? I do not see anything evil about that. And rfc allows it.
>Why should I accept a packet which is most likely an artificially
>crafted packet (RFC 3514)?
>
>Cheers,
>Eldad
>
^ permalink raw reply
* [PATCH net 1/1] RFC: drivers/net/phy: Fix for a BCM5482S auto-negotion problem
From: Corey Ashford @ 2013-09-07 21:04 UTC (permalink / raw)
To: netdev; +Cc: Corey Ashford
When a 1Gb network interface is used in SGMII mode off, and then
a Broadcom 5482S is used to redrive SGMII (SGMII to SGMII mode)
to a Broadcom 54616 PHY, the standard PHY registers do not appear to
be updated correctly after auto-negotiation. This causes the
kernel to get confused about the state of the link and also
causes the MAC layer driver to inappropriately configure the MAC.
By 'standard' registers I mean those that are read by genphy_read_link
(MII_BMSR, 0x01) and genphy_read_status (MII_STAT1000, 0x0a; MII_CTRL1000,
0x09; MII_LPA, 0x05).
Here are the register dumps for the various configurations:
SGMII-to-SGMII mode, 1Gb, full duplex:
# /var/dump1GPhy
Shadow register '11111'=0x00007E5C
Dumping registers for PHY at port e:
PHY register 0=0x00001140
PHY register 1=0x00007949
PHY register 2=0x00000143
PHY register 3=0x0000BCB2
PHY register 4=0x000001E1
PHY register 5=0x00000000
PHY register 6=0x00000064
PHY register 7=0x00002001
PHY register 8=0x00000000
PHY register 9=0x00000200
PHY register A=0x00000000
PHY register B=0x00000000
PHY register C=0x00000000
PHY register D=0x00000000
PHY register E=0x00000000
PHY register F=0x00003000
PHY register 10=0x00001000
PHY register 11=0x00002000
PHY register 12=0x00000000
PHY register 13=0x00000C00
PHY register 14=0x00000000
PHY register 15=0x00000000
PHY register 16=0x00000000
PHY register 17=0x00000000
PHY register 18=0x00000400
PHY register 19=0x00001000
PHY register 1A=0x00000000
PHY register 1B=0x0000FFF1
PHY register 1C=0x00007E5C
PHY register 1D=0x00000000
PHY register 1E=0x00000000
PHY register 1F=0x00000000
PHY expansion register E00=0x00001140
PHY expansion register E01=0x0000016D
PHY expansion register E02=0x00000143
PHY expansion register E03=0x0000BCB2
PHY expansion register E04=0x00000001
PHY expansion register E05=0x0000D801
PHY expansion register E06=0x00000064
PHY expansion register E07=0x00002001
PHY expansion register E08=0x00000000
PHY expansion register E09=0x00000000
PHY expansion register E0A=0x00000000
PHY expansion register E0B=0x00000000
PHY expansion register E0C=0x00000000
PHY expansion register E0D=0x00000000
PHY expansion register E0E=0x00000000
PHY expansion register E0F=0x0000C000
PHY expansion register E10=0x00000000
PHY expansion register E11=0x00000000
PHY expansion register E12=0x00000080
PHY expansion register E13=0x00000089
PHY expansion register E14=0x00000000
PHY expansion register E15=0x0000038A
PHY expansion register E16=0x0000002E
Mode status register = 0x0000D072
Successfully completed!
SGMII-to-SGMII mode, 100Mb, half duplex:
# /var/dump1GPhy
Shadow register '11111'=0x00007E5C
Dumping registers for PHY at port e:
PHY register 0=0x00001140
PHY register 1=0x00007949
PHY register 2=0x00000143
PHY register 3=0x0000BCB2
PHY register 4=0x000001E1
PHY register 5=0x00000000
PHY register 6=0x00000064
PHY register 7=0x00002001
PHY register 8=0x00000000
PHY register 9=0x00000200
PHY register A=0x00000000
PHY register B=0x00000000
PHY register C=0x00000000
PHY register D=0x00000000
PHY register E=0x00000000
PHY register F=0x00003000
PHY register 10=0x00001000
PHY register 11=0x00002000
PHY register 12=0x00000000
PHY register 13=0x00000C00
PHY register 14=0x00000000
PHY register 15=0x00000000
PHY register 16=0x00000000
PHY register 17=0x00000000
PHY register 18=0x00000400
PHY register 19=0x00001000
PHY register 1A=0x00000000
PHY register 1B=0x0000FFF1
PHY register 1C=0x00007E5C
PHY register 1D=0x00000000
PHY register 1E=0x00000000
PHY register 1F=0x00000000
PHY expansion register E00=0x00001140
PHY expansion register E01=0x00000169
PHY expansion register E02=0x00000143
PHY expansion register E03=0x0000BCB2
PHY expansion register E04=0x00000001
PHY expansion register E05=0x0000C401
PHY expansion register E06=0x00000066
PHY expansion register E07=0x00002001
PHY expansion register E08=0x00000000
PHY expansion register E09=0x00000000
PHY expansion register E0A=0x00000000
PHY expansion register E0B=0x00000000
PHY expansion register E0C=0x00000000
PHY expansion register E0D=0x00000000
PHY expansion register E0E=0x00000000
PHY expansion register E0F=0x0000C000
PHY expansion register E10=0x00000000
PHY expansion register E11=0x00000000
PHY expansion register E12=0x00000080
PHY expansion register E13=0x00000058
PHY expansion register E14=0x00000000
PHY expansion register E15=0x0000026A
PHY expansion register E16=0x0000002E
Mode status register = 0x0000A072
Successfully completed!
SGMII-to-serdes, 1Gb, full duplex:
# /var/dump1GPhy
Shadow register '11111'=0x00007E5C
Dumping registers for PHY at port e:
PHY register 0=0x00001140
PHY register 1=0x00007949
PHY register 2=0x00000143
PHY register 3=0x0000BCB2
PHY register 4=0x000001E1
PHY register 5=0x00000000
PHY register 6=0x00000064
PHY register 7=0x00002001
PHY register 8=0x00000000
PHY register 9=0x00000200
PHY register A=0x00000000
PHY register B=0x00000000
PHY register C=0x00000000
PHY register D=0x00000000
PHY register E=0x00000000
PHY register F=0x00003000
PHY register 10=0x00001000
PHY register 11=0x00000000
PHY register 12=0x00000000
PHY register 13=0x00000C00
PHY register 14=0x00000000
PHY register 15=0x0000D072
PHY register 16=0x00000000
PHY register 17=0x00000F42
PHY register 18=0x00000400
PHY register 19=0x00001000
PHY register 1A=0x00000000
PHY register 1B=0x0000FFF1
PHY register 1C=0x00007E5C
PHY register 1D=0x00000000
PHY register 1E=0x00000000
PHY register 1F=0x00000000
PHY expansion register E00=0x00001140
PHY expansion register E01=0x0000014D
PHY expansion register E02=0x00000143
PHY expansion register E03=0x0000BCB2
PHY expansion register E04=0x00000060
PHY expansion register E05=0x00000000
PHY expansion register E06=0x00000064
PHY expansion register E07=0x00002001
PHY expansion register E08=0x00000000
PHY expansion register E09=0x00000000
PHY expansion register E0A=0x00000000
PHY expansion register E0B=0x00000000
PHY expansion register E0C=0x00000000
PHY expansion register E0D=0x00000000
PHY expansion register E0E=0x00000000
PHY expansion register E0F=0x0000C000
PHY expansion register E10=0x00000000
PHY expansion register E11=0x00000000
PHY expansion register E12=0x00000080
PHY expansion register E13=0x000002D3
PHY expansion register E14=0x00000000
PHY expansion register E15=0x00000388
PHY expansion register E16=0x0000002E
Mode status register = 0x0000D072
Successfully completed!
Signed-off-by: Corey Ashford <cjashfor@linux.vnet.ibm.com>
---
drivers/net/phy/broadcom.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index f8c90ea..9f5d076 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -142,6 +142,13 @@
#define MII_BCM54XX_EXP_EXP96_MYST 0x0010
#define MII_BCM54XX_EXP_EXP97 0x0f97
#define MII_BCM54XX_EXP_EXP97_MYST 0x0c0c
+#define MII_BCM54XX_EXP_OPER_MODE (MII_BCM54XX_EXP_SEL_ER | 0x42)
+#define MII_BCM54XX_EXP_OPER_MODE_SERDES_LINK 0x8000
+#define MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_MASK 0x6000
+#define MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_1000 0x4000
+#define MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_100 0x2000
+#define MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_10 0x0000
+#define MII_BCM54XX_EXP_OPER_MODE_SERDES_DUPLEX 0x1000
/*
* BCM5482: Secondary SerDes registers
@@ -491,21 +498,31 @@ static int bcm5482_config_init(struct phy_device *phydev)
static int bcm5482_read_status(struct phy_device *phydev)
{
int err;
+ err = bcm54xx_exp_read(phydev, MII_BCM54XX_EXP_OPER_MODE);
+ if (err < 0)
+ return err;
- err = genphy_read_status(phydev);
+ phydev->link = ((err & MII_BCM54XX_EXP_OPER_MODE_SERDES_LINK) ==
+ MII_BCM54XX_EXP_OPER_MODE_SERDES_LINK);
- if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) {
- /*
- * Only link status matters for 1000Base-X mode, so force
- * 1000 Mbit/s full-duplex status
- */
- if (phydev->link) {
+ if (phydev->link) {
+ switch (err & MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_MASK) {
+ case MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_1000:
phydev->speed = SPEED_1000;
- phydev->duplex = DUPLEX_FULL;
+ break;
+ case MII_BCM54XX_EXP_OPER_MODE_SERDES_SPEED_100:
+ phydev->speed = SPEED_100;
+ break;
+ default:
+ phydev->speed = SPEED_10;
+ break;
}
+ if (err & MII_BCM54XX_EXP_OPER_MODE_SERDES_DUPLEX)
+ phydev->duplex = DUPLEX_FULL;
+ else
+ phydev->duplex = DUPLEX_HALF;
}
-
- return err;
+ return 0;
}
static int bcm54xx_ack_interrupt(struct phy_device *phydev)
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Michio Honda @ 2013-09-07 20:21 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, linux-sctp, Neil Horman
In-Reply-To: <522B8A45.4030005@redhat.com>
I understand, thanks!
Cheers,
- Michio
On Sep 7, 2013, at 10:19 PM, Daniel Borkmann wrote:
> On 09/07/2013 09:40 PM, Michio Honda wrote:
>> Hi,
>>
>> Sorry for that I didn't respond to that warning.
>> You are right, laddr == NULL && addrcnt == 1 is the indicator of the function called by
>> asconf_mgmt().
>>
>> Since your patch is actually redundant, I would suggest putting comment on the
>> line of "if ((laddr == NULL) && (addrcnt == 1)) {", and/or on the checking in your patch.
>
> I think as is is just fine. One can read through the Git log and then see
> the rationale behind a change (which is in most cases even more worth than
> a comment). If this function should ever be called from somewhere else for
> whatever reason, then this comment would already be obsolete.
>
^ permalink raw reply
* Re: [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Daniel Borkmann @ 2013-09-07 20:19 UTC (permalink / raw)
To: Michio Honda; +Cc: davem, netdev, linux-sctp, Neil Horman
In-Reply-To: <B8C81C0E-1D13-40A9-A39F-4E5C8069366D@sfc.wide.ad.jp>
On 09/07/2013 09:40 PM, Michio Honda wrote:
> Hi,
>
> Sorry for that I didn't respond to that warning.
> You are right, laddr == NULL && addrcnt == 1 is the indicator of the function called by
> asconf_mgmt().
>
> Since your patch is actually redundant, I would suggest putting comment on the
> line of "if ((laddr == NULL) && (addrcnt == 1)) {", and/or on the checking in your patch.
I think as is is just fine. One can read through the Git log and then see
the rationale behind a change (which is in most cases even more worth than
a comment). If this function should ever be called from somewhere else for
whatever reason, then this comment would already be obsolete.
^ permalink raw reply
* Re: [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Michio Honda @ 2013-09-07 20:19 UTC (permalink / raw)
To: Neil Horman; +Cc: Daniel Borkmann, davem, netdev, linux-sctp
In-Reply-To: <20130907201128.GA15351@neilslaptop.think-freely.org>
On Sep 7, 2013, at 10:11 PM, Neil Horman wrote:
> On Sat, Sep 07, 2013 at 09:40:15PM +0200, Michio Honda wrote:
>> Hi,
>>
>> Sorry for that I didn't respond to that warning.
>> You are right, laddr == NULL && addrcnt == 1 is the indicator of the function called by
>> asconf_mgmt().
>>
>> Since your patch is actually redundant, I would suggest putting comment on the
>> line of "if ((laddr == NULL) && (addrcnt == 1)) {", and/or on the checking in your patch.
>>
> How can you guarantee its redundant, it seems possible to me to have an
> association for which the laddr might not be found (the NULL case) while having
> a multientry bind list, leading to a NULL dereference? I think we need the
> check.
I meant that laddr == NULL && addrcnt > 1 doesn't happen as Daniel said - laddr == NULL
means the deleting address is the last one, so sctp_bindx_rem() fails before this, and
sctp_asconf_mgmt() always passes addrcnt == 1.
I agree with that using this as an indicator of asconf_del_ip() called from
sctp_asconf_mgmt() is error prone, so I agree with that patch.
I just suggesting putting a comment that explains why we put the check in that
patch.
Cheers,
- Michio
>
> Or do you mean to indicate that checkout laddr == NULL & addrcnt == 1 is
> actually redundant. If so, where is the redundant check?
> Neil
>
>> Cheers,
>> - Michio
>>
>> On Sep 7, 2013, at 8:51 PM, Daniel Borkmann wrote:
>>
>>> This was originally reported in [1] and posted by Neil Horman [2], he said:
>>>
>>> Fix up a missed null pointer check in the asconf code. If we don't find
>>> a local address, but we pass in an address length of more than 1, we may
>>> dereference a NULL laddr pointer. Currently this can't happen, as the only
>>> users of the function pass in the value 1 as the addrcnt parameter, but
>>> its not hot path, and it doesn't hurt to check for NULL should that ever
>>> be the case.
>>>
>>> The callpath from sctp_asconf_mgmt() looks okay. But this could be triggered
>>> from sctp_setsockopt_bindx() call with SCTP_BINDX_REM_ADDR and addrcnt > 1
>>> while passing all possible addresses from the bind list to SCTP_BINDX_REM_ADDR
>>> so that we do *not* find a single address in the association's bind address
>>> list that is not in the packed array of addresses. If this happens when we
>>> have an established association with ASCONF-capable peers, then we could get
>>> a NULL pointer dereference as we only check for laddr == NULL && addrcnt == 1
>>> and call later sctp_make_asconf_update_ip() with NULL laddr.
>>>
>>> BUT: this actually won't happen as sctp_bindx_rem() will catch such a case
>>> and return with an error earlier. As this is incredably unintuitive and error
>>> prone, add a check to catch at least future bugs here. As Neil says, its not
>>> hot path. Introduced by 8a07eb0a5 ("sctp: Add ASCONF operation on the
>>> single-homed host").
>>>
>>> [1] http://www.spinics.net/lists/linux-sctp/msg02132.html
>>> [2] http://www.spinics.net/lists/linux-sctp/msg02133.html
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>>> Cc: Michio Honda <micchie@sfc.wide.ad.jp>
>>> ---
>>> net/sctp/socket.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index 5462bbb..911b71b 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -806,6 +806,9 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
>>> goto skip_mkasconf;
>>> }
>>>
>>> + if (laddr == NULL)
>>> + return -EINVAL;
>>> +
>>> /* We do not need RCU protection throughout this loop
>>> * because this is done under a socket lock from the
>>> * setsockopt call.
>>> --
>>> 1.7.11.7
>>>
>>
>>
^ permalink raw reply
* Re: [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Neil Horman @ 2013-09-07 20:12 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, linux-sctp, Michio Honda
In-Reply-To: <1378579881-27881-1-git-send-email-dborkman@redhat.com>
On Sat, Sep 07, 2013 at 08:51:21PM +0200, Daniel Borkmann wrote:
> This was originally reported in [1] and posted by Neil Horman [2], he said:
>
> Fix up a missed null pointer check in the asconf code. If we don't find
> a local address, but we pass in an address length of more than 1, we may
> dereference a NULL laddr pointer. Currently this can't happen, as the only
> users of the function pass in the value 1 as the addrcnt parameter, but
> its not hot path, and it doesn't hurt to check for NULL should that ever
> be the case.
>
> The callpath from sctp_asconf_mgmt() looks okay. But this could be triggered
> from sctp_setsockopt_bindx() call with SCTP_BINDX_REM_ADDR and addrcnt > 1
> while passing all possible addresses from the bind list to SCTP_BINDX_REM_ADDR
> so that we do *not* find a single address in the association's bind address
> list that is not in the packed array of addresses. If this happens when we
> have an established association with ASCONF-capable peers, then we could get
> a NULL pointer dereference as we only check for laddr == NULL && addrcnt == 1
> and call later sctp_make_asconf_update_ip() with NULL laddr.
>
> BUT: this actually won't happen as sctp_bindx_rem() will catch such a case
> and return with an error earlier. As this is incredably unintuitive and error
> prone, add a check to catch at least future bugs here. As Neil says, its not
> hot path. Introduced by 8a07eb0a5 ("sctp: Add ASCONF operation on the
> single-homed host").
>
> [1] http://www.spinics.net/lists/linux-sctp/msg02132.html
> [2] http://www.spinics.net/lists/linux-sctp/msg02133.html
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Michio Honda <micchie@sfc.wide.ad.jp>
Acked-By: Neil Horman <nhorman@tuxdriver.com>
> ---
> net/sctp/socket.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 5462bbb..911b71b 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -806,6 +806,9 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
> goto skip_mkasconf;
> }
>
> + if (laddr == NULL)
> + return -EINVAL;
> +
> /* We do not need RCU protection throughout this loop
> * because this is done under a socket lock from the
> * setsockopt call.
> --
> 1.7.11.7
>
>
^ permalink raw reply
* Re: [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Neil Horman @ 2013-09-07 20:11 UTC (permalink / raw)
To: Michio Honda; +Cc: Daniel Borkmann, davem, netdev, linux-sctp
In-Reply-To: <B8C81C0E-1D13-40A9-A39F-4E5C8069366D@sfc.wide.ad.jp>
On Sat, Sep 07, 2013 at 09:40:15PM +0200, Michio Honda wrote:
> Hi,
>
> Sorry for that I didn't respond to that warning.
> You are right, laddr == NULL && addrcnt == 1 is the indicator of the function called by
> asconf_mgmt().
>
> Since your patch is actually redundant, I would suggest putting comment on the
> line of "if ((laddr == NULL) && (addrcnt == 1)) {", and/or on the checking in your patch.
>
How can you guarantee its redundant, it seems possible to me to have an
association for which the laddr might not be found (the NULL case) while having
a multientry bind list, leading to a NULL dereference? I think we need the
check.
Or do you mean to indicate that checkout laddr == NULL & addrcnt == 1 is
actually redundant. If so, where is the redundant check?
Neil
> Cheers,
> - Michio
>
> On Sep 7, 2013, at 8:51 PM, Daniel Borkmann wrote:
>
> > This was originally reported in [1] and posted by Neil Horman [2], he said:
> >
> > Fix up a missed null pointer check in the asconf code. If we don't find
> > a local address, but we pass in an address length of more than 1, we may
> > dereference a NULL laddr pointer. Currently this can't happen, as the only
> > users of the function pass in the value 1 as the addrcnt parameter, but
> > its not hot path, and it doesn't hurt to check for NULL should that ever
> > be the case.
> >
> > The callpath from sctp_asconf_mgmt() looks okay. But this could be triggered
> > from sctp_setsockopt_bindx() call with SCTP_BINDX_REM_ADDR and addrcnt > 1
> > while passing all possible addresses from the bind list to SCTP_BINDX_REM_ADDR
> > so that we do *not* find a single address in the association's bind address
> > list that is not in the packed array of addresses. If this happens when we
> > have an established association with ASCONF-capable peers, then we could get
> > a NULL pointer dereference as we only check for laddr == NULL && addrcnt == 1
> > and call later sctp_make_asconf_update_ip() with NULL laddr.
> >
> > BUT: this actually won't happen as sctp_bindx_rem() will catch such a case
> > and return with an error earlier. As this is incredably unintuitive and error
> > prone, add a check to catch at least future bugs here. As Neil says, its not
> > hot path. Introduced by 8a07eb0a5 ("sctp: Add ASCONF operation on the
> > single-homed host").
> >
> > [1] http://www.spinics.net/lists/linux-sctp/msg02132.html
> > [2] http://www.spinics.net/lists/linux-sctp/msg02133.html
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > Cc: Michio Honda <micchie@sfc.wide.ad.jp>
> > ---
> > net/sctp/socket.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 5462bbb..911b71b 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -806,6 +806,9 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
> > goto skip_mkasconf;
> > }
> >
> > + if (laddr == NULL)
> > + return -EINVAL;
> > +
> > /* We do not need RCU protection throughout this loop
> > * because this is done under a socket lock from the
> > * setsockopt call.
> > --
> > 1.7.11.7
> >
>
>
^ permalink raw reply
* Re: [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Michio Honda @ 2013-09-07 19:40 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, linux-sctp, Neil Horman
In-Reply-To: <1378579881-27881-1-git-send-email-dborkman@redhat.com>
Hi,
Sorry for that I didn't respond to that warning.
You are right, laddr == NULL && addrcnt == 1 is the indicator of the function called by
asconf_mgmt().
Since your patch is actually redundant, I would suggest putting comment on the
line of "if ((laddr == NULL) && (addrcnt == 1)) {", and/or on the checking in your patch.
Cheers,
- Michio
On Sep 7, 2013, at 8:51 PM, Daniel Borkmann wrote:
> This was originally reported in [1] and posted by Neil Horman [2], he said:
>
> Fix up a missed null pointer check in the asconf code. If we don't find
> a local address, but we pass in an address length of more than 1, we may
> dereference a NULL laddr pointer. Currently this can't happen, as the only
> users of the function pass in the value 1 as the addrcnt parameter, but
> its not hot path, and it doesn't hurt to check for NULL should that ever
> be the case.
>
> The callpath from sctp_asconf_mgmt() looks okay. But this could be triggered
> from sctp_setsockopt_bindx() call with SCTP_BINDX_REM_ADDR and addrcnt > 1
> while passing all possible addresses from the bind list to SCTP_BINDX_REM_ADDR
> so that we do *not* find a single address in the association's bind address
> list that is not in the packed array of addresses. If this happens when we
> have an established association with ASCONF-capable peers, then we could get
> a NULL pointer dereference as we only check for laddr == NULL && addrcnt == 1
> and call later sctp_make_asconf_update_ip() with NULL laddr.
>
> BUT: this actually won't happen as sctp_bindx_rem() will catch such a case
> and return with an error earlier. As this is incredably unintuitive and error
> prone, add a check to catch at least future bugs here. As Neil says, its not
> hot path. Introduced by 8a07eb0a5 ("sctp: Add ASCONF operation on the
> single-homed host").
>
> [1] http://www.spinics.net/lists/linux-sctp/msg02132.html
> [2] http://www.spinics.net/lists/linux-sctp/msg02133.html
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Michio Honda <micchie@sfc.wide.ad.jp>
> ---
> net/sctp/socket.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 5462bbb..911b71b 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -806,6 +806,9 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
> goto skip_mkasconf;
> }
>
> + if (laddr == NULL)
> + return -EINVAL;
> +
> /* We do not need RCU protection throughout this loop
> * because this is done under a socket lock from the
> * setsockopt call.
> --
> 1.7.11.7
>
^ permalink raw reply
* Re: [PATCH net] net: fib: fib6_add: fix potential NULL pointer dereference
From: Hannes Frederic Sowa @ 2013-09-07 19:35 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Lin Ming, Matti Vaittinen
In-Reply-To: <1378559600-15858-1-git-send-email-dborkman@redhat.com>
On Sat, Sep 07, 2013 at 03:13:20PM +0200, Daniel Borkmann wrote:
> When the kernel is compiled with CONFIG_IPV6_SUBTREES, and we return
> with an error in fn = fib6_add_1(), then error codes are encoded into
> the return pointer e.g. ERR_PTR(-ENOENT). In such an error case, we
> write the error code into err and jump to out, hence enter the if(err)
> condition. Now, if CONFIG_IPV6_SUBTREES is enabled, we check for:
>
> if (pn != fn && pn->leaf == rt)
> ...
> if (pn != fn && !pn->leaf && !(pn->fn_flags & RTN_RTINFO))
> ...
>
> Since pn is NULL and fn is f.e. ERR_PTR(-ENOENT), then pn != fn
> evaluates to true and causes a NULL-pointer dereference on further
> checks on pn. Fix it, by setting both NULL in error case, so that
> pn != fn already evaluates to false and no further dereference
> takes place.
>
> This was first correctly implemented in 4a287eba2 ("IPv6 routing,
> NLM_F_* flag support: REPLACE and EXCL flags support, warn about
> missing CREATE flag"), but the bug got later on introduced by
> 188c517a0 ("ipv6: return errno pointers consistently for fib6_add_1()").
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Lin Ming <mlin@ss.pku.edu.cn>
> Cc: Matti Vaittinen <matti.vaittinen@nsn.com>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Full ACK!
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
^ permalink raw reply
* Re: Realtek r8168 hangs when sending data at full speed on a gigabit link
From: Frédéric Leroy @ 2013-09-07 19:35 UTC (permalink / raw)
To: Francois Romieu
Cc: netdev, Realtek linux nic maintainers, David R, Hayes Wang
In-Reply-To: <20130907101546.GA19560@electric-eye.fr.zoreil.com>
Hello,
Le 07/09/2013 12:15, Francois Romieu a écrit :
> Frédéric Leroy <fredo@starox.org> :
> [...]
>
> Sorry for the delay. It was a busy week.
>
> Can you give the hack below a try ?
I tested it with and without.
The patch works perfectly ! Thanks :)
--
Frédéric
^ permalink raw reply
* Re: [net v6 1/8] i40e: main driver core
From: Brandeburg, Jesse @ 2013-09-07 19:27 UTC (permalink / raw)
To: Daniel Borkmann
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
gospo@redhat.com, davem@davemloft.net, sassmann@redhat.com
In-Reply-To: <522AD28B.5050507@redhat.com>
On Sat, 2013-09-07 at 09:15 +0200, Daniel Borkmann wrote:
> Thanks for including SCTP! ;-)
:-)
> Here as well, I40E_ERR_NO_MEMORY vs -ENOMEM.
>
okay we will audit those and fix them.
>
> Nitpick: why s32 in the signature? There are a lot of such places, just int would have
> been fine probably.
>
agreed.
> }
> > + if (pf->lan_veb == I40E_NO_VEB) {
> > + v = i40e_veb_mem_alloc(pf);
> > + if (v < 0)
> > + break;
>
> Nitpick: I'd expect from *alloc() functions to return NULL, but fair enough.
I will take a look at what we can do about these.
> > + aq_buf = kzalloc(I40E_AQ_LARGE_BUF, GFP_KERNEL);
> > + if (!aq_buf)
> > + return -ENOMEM;
>
> More of such examples ... ;-) I40E_ERR_BAD_PTR vs. -ENOMEM on a s32 instead of int.
right
> > + } else if (pf->flags & I40E_FLAG_RSS_ENABLED &&
> > + !(pf->flags & I40E_FLAG_FDIR_ENABLED) &&
> > + !(pf->flags & I40E_FLAG_DCB_ENABLED)) {
> > +
> > + SET_RSS_SIZE;
>
> Can't these macros be done in a small inline function instead?
I'll take a look, as well for all instances of this.
Thanks again,
Jesse
------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58041391&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [net v6 1/8] i40e: main driver core
From: Brandeburg, Jesse @ 2013-09-07 19:20 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com, Nelson, Shannon,
Waskiewicz Jr, Peter P, e1000-devel@lists.sourceforge.net
In-Reply-To: <522ACB54.2050706@redhat.com>
On Sat, 2013-09-07 at 08:44 +0200, Daniel Borkmann wrote:
> Nitpicking ... at some point in time i40e_status should be removed plus
> I40E_ERR_PARAM, I40E_SUCCESS, I40E_ERR_NO_MEMORY and the like, as we have
> int and -EINVAL, 0, -ENOMEM for that. ;-)
First, thanks Daniel for taking the time to review.
Those are a result of our files that are shared across OSes, as not all
OSes have -ENOMEM etc, we also have a lot of status codes the kernel
doesn't have. That said, when there is a 1-1 relationship the
replacements should be made.
>
> [...]
> +i40e_status i40e_put_mac_in_vlan(struct i40e_vsi *vsi, u8 *macaddr,
> + bool is_vf, bool is_netdev)
> +{
> + struct i40e_mac_filter *f, *add_f;
> +
> + list_for_each_entry(f, &vsi->mac_filter_list, list) {
> [...]
> + if (!add_f) {
> + dev_info(&vsi->back->pdev->dev, "Could not add filter %d for %pM\n",
> + f->vlan, f->macaddr);
> + return -ENOMEM;
> + }
> + }
> + }
> + return I40E_SUCCESS;
> +}
>
> Their usage seems also to be mixed anyway: -ENOMEM vs. I40E_SUCCESS.
that's just plain buggy.
> [...]
> +void i40e_vsi_reset_stats(struct i40e_vsi *vsi)
> +{
> + struct rtnl_link_stats64 *ns;
> + int i;
> +
> + if (!vsi)
> + return;
> +
> [...]
> +static struct i40e_mac_filter *i40e_find_filter(struct i40e_vsi *vsi,
> + u8 *macaddr, s16 vlan,
> + bool is_vf, bool is_netdev)
> +{
> + struct i40e_mac_filter *f;
> +
> + if (!vsi || !macaddr)
> + return NULL;
> [...]
>
> Probably the code could also be scanned to remove such checks as well ...
do you mean we should not be checking for NULL vsi or NULL macaddr? We
have several structures in the driver that by design don't have all
elements filled in for certain cases.
I went and looked at the !vsi case and we can definitely remove that in
most cases, as there are several nested checks for null vsi.
thanks again,
Jesse
^ permalink raw reply
* [PATCH] net: fix multiqueue selection
From: Eric Dumazet @ 2013-09-07 19:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Alexander Duyck
From: Eric Dumazet <edumazet@google.com>
commit 416186fbf8c5b4e4465 ("net: Split core bits of netdev_pick_tx
into __netdev_pick_tx") added a bug that disables caching of queue
index in the socket.
This is the source of packet reorders for TCP flows, and
again this is happening more often when using FQ pacing.
Old code was doing
if (queue_index != old_index)
sk_tx_queue_set(sk, queue_index);
Alexander renamed the variables but forgot to change sk_tx_queue_set()
2nd parameter.
if (queue_index != new_index)
sk_tx_queue_set(sk, queue_index);
This means we store -1 over and over in sk->sk_tx_queue_mapping
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
---
net/core/flow_dissector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 0ff42f0..1929af8 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -352,7 +352,7 @@ u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb)
if (queue_index != new_index && sk &&
rcu_access_pointer(sk->sk_dst_cache))
- sk_tx_queue_set(sk, queue_index);
+ sk_tx_queue_set(sk, new_index);
queue_index = new_index;
}
^ permalink raw reply related
* [PATCH net] net: sctp: fix smatch warning in sctp_send_asconf_del_ip
From: Daniel Borkmann @ 2013-09-07 18:51 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-sctp, Neil Horman, Michio Honda
This was originally reported in [1] and posted by Neil Horman [2], he said:
Fix up a missed null pointer check in the asconf code. If we don't find
a local address, but we pass in an address length of more than 1, we may
dereference a NULL laddr pointer. Currently this can't happen, as the only
users of the function pass in the value 1 as the addrcnt parameter, but
its not hot path, and it doesn't hurt to check for NULL should that ever
be the case.
The callpath from sctp_asconf_mgmt() looks okay. But this could be triggered
from sctp_setsockopt_bindx() call with SCTP_BINDX_REM_ADDR and addrcnt > 1
while passing all possible addresses from the bind list to SCTP_BINDX_REM_ADDR
so that we do *not* find a single address in the association's bind address
list that is not in the packed array of addresses. If this happens when we
have an established association with ASCONF-capable peers, then we could get
a NULL pointer dereference as we only check for laddr == NULL && addrcnt == 1
and call later sctp_make_asconf_update_ip() with NULL laddr.
BUT: this actually won't happen as sctp_bindx_rem() will catch such a case
and return with an error earlier. As this is incredably unintuitive and error
prone, add a check to catch at least future bugs here. As Neil says, its not
hot path. Introduced by 8a07eb0a5 ("sctp: Add ASCONF operation on the
single-homed host").
[1] http://www.spinics.net/lists/linux-sctp/msg02132.html
[2] http://www.spinics.net/lists/linux-sctp/msg02133.html
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Michio Honda <micchie@sfc.wide.ad.jp>
---
net/sctp/socket.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5462bbb..911b71b 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -806,6 +806,9 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
goto skip_mkasconf;
}
+ if (laddr == NULL)
+ return -EINVAL;
+
/* We do not need RCU protection throughout this loop
* because this is done under a socket lock from the
* setsockopt call.
--
1.7.11.7
^ permalink raw reply related
* Re: Realtek r8168 hangs when sending data at full speed on a gigabit link
From: David R @ 2013-09-07 17:19 UTC (permalink / raw)
To: Francois Romieu
Cc: Frédéric Leroy, netdev, Realtek linux nic maintainers,
Hayes Wang
In-Reply-To: <20130907101546.GA19560@electric-eye.fr.zoreil.com>
Hi
You mean this line from the dmesg? :-
[ 6.015979] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
[ 6.016285] r8169 0000:02:00.0: irq 73 for MSI/MSI-X
[ 6.016549] r8169 0000:02:00.0 eth0: RTL8168f/8111f at
0xffffc9000060e000, 60:a4:4c:2c:ff:a1, XID 08000800 IRQ 73
Cheers
David
On 07/09/13 11:15, Francois Romieu wrote:
> Frédéric Leroy <fredo@starox.org> :
> [...]
>
> Sorry for the delay. It was a busy week.
>
> Can you give the hack below a try ?
>
> David, could you send me the r8169 XID line from a kernel running on
> the hardware for which I sent you a similar patch back in 2013/04 ?
> You appeared to own a 8168f and it could be a RTL_GIGA_MAC_VER_36.
>
> Thanks.
>
> Hayes, see http://marc.info/?l=linux-netdev&m=137794473416308&w=1 for
> history. It could be eb2dc35d99028b698cdedba4f5522bc43e576bd2
> ("r8169: RxConfig hack for the 8168evl.") return, with a revenge.
>
> ---
> drivers/net/ethernet/realtek/r8169.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 6f87f2c..3397cee 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4231,6 +4231,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
> case RTL_GIGA_MAC_VER_23:
> case RTL_GIGA_MAC_VER_24:
> case RTL_GIGA_MAC_VER_34:
> + case RTL_GIGA_MAC_VER_35:
> RTL_W32(RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST);
> break;
> case RTL_GIGA_MAC_VER_40:
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Eric Dumazet @ 2013-09-07 17:21 UTC (permalink / raw)
To: Zoltan Kiss; +Cc: Wei Liu, Jonathan Davies, Ian Campbell, netdev, xen-devel
In-Reply-To: <1378486840.31445.36.camel@edumazet-glaptop>
On Fri, 2013-09-06 at 10:00 -0700, Eric Dumazet wrote:
> On Fri, 2013-09-06 at 17:36 +0100, Zoltan Kiss wrote:
>
> > So I guess it would be good to revisit the default value of this
> > setting.
>
> If ixgbe requires 3 TSO packets in TX ring to get line rate, you also
> can tweak dev->gso_max_size from 65535 to 64000.
Another idea would be to no longer use tcp_limit_output_bytes but
max(sk_pacing_rate / 1000, 2*MSS)
This means that number of packets in FQ would be limited to the
equivalent of 1ms, so TCP could have faster response to packet losses :
Retransmitted packets would not have to wait for prior packets being
drained from FQ
For a 8Gbps flow, 1Gbyte/s, sk_pacing_rate would be 2Gbyte, this would
translate to ~2 Mbytes in Qdisc/TX ring.
sk_pacing_rate was introduced in linux-3.12, but could be backported
easily.
^ permalink raw reply
* Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: Eldad Zack @ 2013-09-07 16:46 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <20130907154602.GA1442@minipsycho.orion>
On Sat, 7 Sep 2013, Jiri Pirko wrote:
> Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
> >
> >Hi Jiri,
> >
> >On Fri, 6 Sep 2013, Jiri Pirko wrote:
> >
> >> In rfc4942 and rfc2460 I cannot find anything which would implicate to
> >> drop packets which have only padding in tlv.
> >
> >NAK from my side.
> >Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
>
> I did.
>
> >
> >While it doesn't specifically discusses this corner case, you can
> >understand from "There is no legitimate reason for padding beyond the
> >next eight octet..." that there's also no legitimate reason for an
> >option header containing only padding.
>
> Okay. I'm glad you agree with me and that we both understand the rfc the
> same way. And since the rfc does not say that "here's no legitimate
> reason for an option header containing only padding", this should be
> possible. I say we respect rfc and do not add stronger restrictions than
> it dictates. No need for them.
Strictly speaking, this RFC is informational, so it is doesn't dictate
per se. I hope you don't suggest removing the other checks as well...
> >I can't imagine a sane use-case for this.
>
> rfcs are not about sanity...
Great, we're in agreement again :) But I think the networking code is
or should be.
What IPv6 stack would generate such a packet, given that the only usage
of the padding options is to align other options?
Why should I accept a packet which is most likely an artificially
crafted packet (RFC 3514)?
Cheers,
Eldad
^ permalink raw reply
* Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: Jiri Pirko @ 2013-09-07 15:46 UTC (permalink / raw)
To: Eldad Zack; +Cc: netdev, davem, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <alpine.LNX.2.00.1309071427120.1262@heraclitus>
Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
>
>Hi Jiri,
>
>On Fri, 6 Sep 2013, Jiri Pirko wrote:
>
>> In rfc4942 and rfc2460 I cannot find anything which would implicate to
>> drop packets which have only padding in tlv.
>
>NAK from my side.
>Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
I did.
>
>While it doesn't specifically discusses this corner case, you can
>understand from "There is no legitimate reason for padding beyond the
>next eight octet..." that there's also no legitimate reason for an
>option header containing only padding.
Okay. I'm glad you agree with me and that we both understand the rfc the
same way. And since the rfc does not say that "here's no legitimate
reason for an option header containing only padding", this should be
possible. I say we respect rfc and do not add stronger restrictions than
it dictates. No need for them.
>I can't imagine a sane use-case for this.
rfcs are not about sanity...
>
>> Current behaviour breaks TAHI Test v6LC.1.2.6.
>
>I'm not familiar with this, but IMHO the test should be reversed :)
>
>Cheers,
>Eldad
>
>>
>> Problem was intruduced in:
>> 9b905fe6843 "ipv6/exthdrs: strict Pad1 and PadN check"
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>> net/ipv6/exthdrs.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
>> index 07a7d65..8d67900 100644
>> --- a/net/ipv6/exthdrs.c
>> +++ b/net/ipv6/exthdrs.c
>> @@ -162,12 +162,6 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
>> off += optlen;
>> len -= optlen;
>> }
>> - /* This case will not be caught by above check since its padding
>> - * length is smaller than 7:
>> - * 1 byte NH + 1 byte Length + 6 bytes Padding
>> - */
>> - if ((padlen == 6) && ((off - skb_network_header_len(skb)) == 8))
>> - goto bad;
>>
>> if (len == 0)
>> return true;
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Veaceslav Falico @ 2013-09-07 15:03 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Netdev
In-Reply-To: <522B3BF1.2020208@redhat.com>
On Sat, Sep 07, 2013 at 04:45:05PM +0200, Nikolay Aleksandrov wrote:
>
>On 09/07/2013 04:20 PM, Veaceslav Falico wrote:
>> On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote:
...snip...
>> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
>> index f4b1001..37b49d1 100644
>> --- a/include/linux/rculist.h
>> +++ b/include/linux/rculist.h
>> @@ -23,6 +23,7 @@
>> * way, we must not access it directly
>> */
>> #define list_next_rcu(list) (*((struct list_head __rcu
>> **)(&(list)->next)))
>> +#define list_prev_rcu(list) (*((struct list_head __rcu
>> **)(&(list)->prev)))
>>
>> /*
>> * Insert a new entry between two known consecutive entries.
>> @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct
>> list_head *list,
>> likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
>> })
>>
>> +#define list_last_or_null_rcu(ptr, type, member) \
>> + ({struct list_head *__ptr = (ptr); \
>> + struct list_head __rcu *__last = list_prev_rcu(__ptr); \
>> + likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \
>> + })
>> +
>Hi,
>Actually I don't think you can dereference ->prev and use the standard
>list_del_rcu because it guarantees only the ->next ptr will be valid and
>->prev is set to LIST_POISON2.
>IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193
>with the bidir_del and all that.
Yeah, right, my bad - we can rely only on the ->next pointer, indeed,
missed that part. RCU is hard :).
So it'll be a lot harder to implement bond_last_slave_rcu() in a
'straightforward' approach.
I'd rather go in the opposite direction here - i.e. drop the 'reverse'
traversal completely, and all the use cases for bond_last_slave_rcu(). I've
got some patches already - http://patchwork.ozlabs.org/patch/272076/ doing
that, and hopefully will remove the whole 'backword' traversal completely
in the future.
>
>But in any case I complete agree with Veaceslav here. Read all the
>documentation carefully :-)
>
>Cheers,
> Nik
>
>> /**
>> * list_for_each_entry_rcu - iterate over rcu list of given type
>> * @pos: the type * to use as a loop cursor.
>> ------- END OF PATCH ------
>>
>> Anyway, it's up to you.
>>
>> Hope that helps.
>
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Nikolay Aleksandrov @ 2013-09-07 14:45 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Netdev
In-Reply-To: <20130907142041.GA20237@redhat.com>
On 09/07/2013 04:20 PM, Veaceslav Falico wrote:
> On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote:
<snip>
> (Also, mind the "likely(__ptr != __next)" - usually, and bonding is a very
> good example, we always have at least one element in the list.)
>
> This way, I'd recommend you to do bond_last_slave_rcu() the same way as
> here - you, though, might omit saving the slave_list pointer, it's not
> needed in case of bonding AFAIK. Something like that (I'm writing it in my
> mail editor - so it's only for the reference):
>
> #define bond_last_slave_rcu(bond) \
> ({struct list_head *__slave_ptr = list_next_rcu(&bond->slave_list); \
> likely(__slave_ptr != &bond->slave_list) ? \
> bond_to_slave_rcu(__slave_ptr) : NULL;})
>
>
> Or, even better (from my POV), add a generic macro to rculist.h (again,
> didn't even compile it) - it can be used later on:
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index f4b1001..37b49d1 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -23,6 +23,7 @@
> * way, we must not access it directly
> */
> #define list_next_rcu(list) (*((struct list_head __rcu
> **)(&(list)->next)))
> +#define list_prev_rcu(list) (*((struct list_head __rcu
> **)(&(list)->prev)))
>
> /*
> * Insert a new entry between two known consecutive entries.
> @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct
> list_head *list,
> likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
> })
>
> +#define list_last_or_null_rcu(ptr, type, member) \
> + ({struct list_head *__ptr = (ptr); \
> + struct list_head __rcu *__last = list_prev_rcu(__ptr); \
> + likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \
> + })
> +
Hi,
Actually I don't think you can dereference ->prev and use the standard
list_del_rcu because it guarantees only the ->next ptr will be valid and
->prev is set to LIST_POISON2.
IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193
with the bidir_del and all that.
But in any case I complete agree with Veaceslav here. Read all the
documentation carefully :-)
Cheers,
Nik
> /**
> * list_for_each_entry_rcu - iterate over rcu list of given type
> * @pos: the type * to use as a loop cursor.
> ------- END OF PATCH ------
>
> Anyway, it's up to you.
>
> Hope that helps.
^ permalink raw reply
* [PATCH net] net: sctp: fix bug in sctp_poll for SOCK_SELECT_ERR_QUEUE
From: Daniel Borkmann @ 2013-09-07 14:44 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-sctp, Jacob Keller
If we do not add braces around ...
mask |= POLLERR |
sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? POLLPRI : 0;
... then this condition always evaluates to true as POLLERR is
defined as 8 and binary or'd with whatever result comes out of
sock_flag(). Hence instead of (X | Y) ? A : B, transform it into
X | (Y ? A : B). Unfortunatelty, commit 8facd5fb73 ("net: fix
smatch warnings inside datagram_poll") forgot about SCTP. :-(
Introduced by 7d4c04fc170 ("net: add option to enable error queue
packets waking select").
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
---
net/sctp/socket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index d5d5882..5462bbb 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -6176,7 +6176,7 @@ unsigned int sctp_poll(struct file *file, struct socket *sock, poll_table *wait)
/* Is there any exceptional events? */
if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
mask |= POLLERR |
- sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? POLLPRI : 0;
+ (sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? POLLPRI : 0);
if (sk->sk_shutdown & RCV_SHUTDOWN)
mask |= POLLRDHUP | POLLIN | POLLRDNORM;
if (sk->sk_shutdown == SHUTDOWN_MASK)
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH net] bnx2x: Restore a call to config_init
From: Eric Dumazet @ 2013-09-07 14:43 UTC (permalink / raw)
To: Dmitry Kravkov
Cc: David Miller, Eilon Greenstein, netdev@vger.kernel.org, davej
In-Reply-To: <CAM8tLiNj-s0RKUeKePDURK7W7HZF=8Zz7D073qcqCLmy75CevQ@mail.gmail.com>
On Sat, 2013-09-07 at 13:35 +0300, Dmitry Kravkov wrote:
> On Sat, Sep 7, 2013 at 1:39 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Oh well, latest net tree broke bnx2x again.
> >
> > No idea why.
> >
> > [ 61.203313] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 74.021238] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[1]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 76.109176] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 78.198178] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[1]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 80.286960] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 93.031584] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[1]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 95.069939] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 97.133493] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[1]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 99.195912] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 112.026026] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[1]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 114.087991] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 116.148976] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[1]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
> > [ 118.237310] bnx2x: [bnx2x_clean_tx_queue:1259(eth0)]timeout waiting for queue[2]: txdata->tx_pkt_prod(1) != txdata->tx_pkt_cons(0)
>
> Unable to reproduce ,,,
> Which HW you have - i will try to get similar...
> Thanks
> --
To have a working NIC, I had to revert
937e5c3 bnx2x: Restore a call to config_init
9b0be65 bnx2x: fix broken compilation with CONFIG_BNX2X_SRIOV is not set
c0a77ec bnx2x: Add missing braces in bnx2x:bnx2x_link_initialize
60cad4e bnx2x: VF RSS support - VF side
b9871bc bnx2x: VF RSS support - PF side
# lspci -s 03:00.0
03:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM57712 10 Gigabit Ethernet (rev 01)
$ grep BNX2X .config
# CONFIG_SCSI_BNX2X_FCOE is not set
CONFIG_BNX2X=m
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Veaceslav Falico @ 2013-09-07 14:20 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
In-Reply-To: <52298407.9040103@huawei.com>
On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote:
...snip...
>+/**
>+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
>+ * Caller must hold rcu_read_lock
>+ */
>+#define bond_first_slave_rcu(bond) \
>+ (bond_is_empty_rcu(bond) ? NULL : \
>+ bond_to_slave_rcu((bond)->slave_list.next))
>+#define bond_last_slave_rcu(bond) \
>+ (bond_is_empty_rcu(bond) ? NULL : \
>+ bond_to_slave_rcu((bond)->slave_list.prev))
>+
Hi Ding,
I didn't have time to actually go into detail about this last week, and, as
Eric correctly said, RCU is hard even for experienced kernel developers, so
it's really not the easiest part to understand. I, for one, can't say that
I understand it completely, though I know the overall, basic usage.
That being said, I really advise you to go through, first of all,
Documentation/RCU. There are also tons of online materials already written
on this topic - and every one takes its own approach in explaining it.
I'll now try to explain what is wrong with the approach of verifying for
list emptyness while holding only RCU read lock.
First of all, you've replied to one of my emails that, quoting - "the
slave_list will not changed in the rcu_read_lock". This is completely wrong
- rcu_read_lock() is not a lock, in its classical meaning. It allows the
list, and everything else, to be *modified* (in the most basic case) while
holding it. I.e. if we, concurrently, run list_for_each_entry_rcu() and, on
another CPU, list_del_rcu() - then we might end up holding a reference to
the entry that *was* deleted from the list already. In other words - the
list can be freely modified while we're holding rcu_read_lock().
What rcu_read_lock() *does* guarantee is that the entry we've dereferenced
won't 'go away' - as in - get freed, get deinitialized (in most cases, and
in this one - with slave_list) or whatever else that can stop us from using
it. That's, again, a really broad assumption and there are *lots* of small
things to be taken care of, and, in some cases it's not even true -
however, the basic idea is this - once you rcu_dereference() something -
you can use it while holding rcu_read_lock().
So, again - while holding the rcu_read_lock(), working with a list - the
list itself can change easily, can become empty, or can become non-empty,
or whatever else - even while going through it via
list_for_each_entry_rcu(). However, if we dereference an entry there - we
can use it.
Now, about the list emptiness under rcu_read_lock(). Lets suppose the
following code (imagine that we have list_empty_rcu(), and we're holding
the rcu_read_lock()):
1 if (!list_empty_rcu(&my_list)) {
2 do_something(get_first_entry(&my_list));
3 }
on the 1st line, we verify if the list is not empty - and, sometimes, it
will be true. For example, we'll have only one element there.
Now, after we execute the first line, on another CPU we *remove* the only
remaining element from the list, and thus my_list->next will point to
&my_list - classical empty list.
So, back to the first CPU, when we run our code, we execute
get_first_entry(&my_list) - which, usually, translates to
container_of(my_list->next, ...) - getting the container of the next
element of the list - which is our *head* - and not a valid entry. And we
will, eventually, bug here.
This is exactly what you're doing:
+#define bond_last_slave_rcu(bond) \
+ (bond_is_empty_rcu(bond) ? NULL : \
+ bond_to_slave_rcu((bond)->slave_list.prev))
You first verify if the list is empty - which can be non-empty *in the time
of the verification* - and then get the bond_to_slave_rcu() of the .prev -
when the list can already become empty and the prev will point to the list
head, and not to a valid slave.
That's exactly why we don't have the list_empty_rcu() - because it's
meaningless - cause exactly after it was run the list can become empty or
non-empty. It guarantees us nothing. The only, maybe, slightly useful usage
of it could be some kind of optimization:
1 rcu_read_lock();
2
3 if (unlikely(list_empty(&bond->slave_list))) {
4 rcu_read_unlock();
5 return -ENODEV;
6 }
7
8 heavy_preparation1(bond);
9 heavy_preparation2(bond);
10 ...
11
12 bond_for_each_slave(bond, slave)
13 do_something_with_a_slave(slave);
14
15 rcu_read_unlock();
Here, as you can see, we must make some heavy (time/memory-consuming)
preparations to the bond *if it has slaves* before working *with each
slave*, and if he doesn't - we can just omit those preparations (if we
can...). But still it's a bit useless, and usually we anyway need proper
RTNL or bond->lock locking before doing something like that.
Back to the real world - to implement the bond_first/last_slave_rcu(), we
*must not* rely on list emptiness before taking the reference to the
first/last element of the list. To make this happen, we *first* take the
reference to list.next/list.prev, and only *after* it we verify if what
we've dereferenced is an actual slave or the list head (i.e. the list was
empty). If it's the list head - then we assume that the list was empty and
return NULL, otherwise - if it's a normal slave - we return the
container_of(what we've dereferenced).
Here's the function that gets the first element in the list:
268 #define list_first_or_null_rcu(ptr, type, member) \
269 ({struct list_head *__ptr = (ptr); \
270 struct list_head __rcu *__next = list_next_rcu(__ptr); \
271 likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
272 })
As you can see, we first make a copy of the list_head pointer, which we
were given - cause in the meanwhile *it can also, possibly, change*.
269 ({struct list_head *__ptr = (ptr); \
Then we dereference the pointer's copy.next, and also save it - cause it
can *also* change - become (non)empty, for example.
270 struct list_head __rcu *__next = list_next_rcu(__ptr); \
And here comes the rcu magic, actually. We know that the ptr can change,
the ptr.next can also change, however we also know that if ptr.next, when
we've dereferenced it, while holding the rcu_read_lock(), was a normal list
element - then we're sure that this element *will not go away*. Thus,
everything what remained for us is just to verify - is our next pointer the
list head or not? I.e. do we actually have an element in __next, or was the
list empty at that time and we can return NULL? And, of course, if the list
was not empty - we have a valid __next, and we can return its container:
271 likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
(Also, mind the "likely(__ptr != __next)" - usually, and bonding is a very
good example, we always have at least one element in the list.)
This way, I'd recommend you to do bond_last_slave_rcu() the same way as
here - you, though, might omit saving the slave_list pointer, it's not
needed in case of bonding AFAIK. Something like that (I'm writing it in my
mail editor - so it's only for the reference):
#define bond_last_slave_rcu(bond) \
({struct list_head *__slave_ptr = list_next_rcu(&bond->slave_list); \
likely(__slave_ptr != &bond->slave_list) ? \
bond_to_slave_rcu(__slave_ptr) : NULL;})
Or, even better (from my POV), add a generic macro to rculist.h (again,
didn't even compile it) - it can be used later on:
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index f4b1001..37b49d1 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -23,6 +23,7 @@
* way, we must not access it directly
*/
#define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
+#define list_prev_rcu(list) (*((struct list_head __rcu **)(&(list)->prev)))
/*
* Insert a new entry between two known consecutive entries.
@@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct list_head *list,
likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
})
+#define list_last_or_null_rcu(ptr, type, member) \
+ ({struct list_head *__ptr = (ptr); \
+ struct list_head __rcu *__last = list_prev_rcu(__ptr); \
+ likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \
+ })
+
/**
* list_for_each_entry_rcu - iterate over rcu list of given type
* @pos: the type * to use as a loop cursor.
------- END OF PATCH ------
Anyway, it's up to you.
Hope that helps.
^ permalink raw reply related
* [PATCH net] net: fib: fib6_add: fix potential NULL pointer dereference
From: Daniel Borkmann @ 2013-09-07 13:13 UTC (permalink / raw)
To: davem; +Cc: netdev, Lin Ming, Matti Vaittinen, Hannes Frederic Sowa
When the kernel is compiled with CONFIG_IPV6_SUBTREES, and we return
with an error in fn = fib6_add_1(), then error codes are encoded into
the return pointer e.g. ERR_PTR(-ENOENT). In such an error case, we
write the error code into err and jump to out, hence enter the if(err)
condition. Now, if CONFIG_IPV6_SUBTREES is enabled, we check for:
if (pn != fn && pn->leaf == rt)
...
if (pn != fn && !pn->leaf && !(pn->fn_flags & RTN_RTINFO))
...
Since pn is NULL and fn is f.e. ERR_PTR(-ENOENT), then pn != fn
evaluates to true and causes a NULL-pointer dereference on further
checks on pn. Fix it, by setting both NULL in error case, so that
pn != fn already evaluates to false and no further dereference
takes place.
This was first correctly implemented in 4a287eba2 ("IPv6 routing,
NLM_F_* flag support: REPLACE and EXCL flags support, warn about
missing CREATE flag"), but the bug got later on introduced by
188c517a0 ("ipv6: return errno pointers consistently for fib6_add_1()").
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Lin Ming <mlin@ss.pku.edu.cn>
Cc: Matti Vaittinen <matti.vaittinen@nsn.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
net/ipv6/ip6_fib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 73db48e..5bec666 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -825,9 +825,9 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt, struct nl_info *info)
fn = fib6_add_1(root, &rt->rt6i_dst.addr, rt->rt6i_dst.plen,
offsetof(struct rt6_info, rt6i_dst), allow_create,
replace_required);
-
if (IS_ERR(fn)) {
err = PTR_ERR(fn);
+ fn = NULL;
goto out;
}
--
1.7.11.7
^ permalink raw reply related
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