* [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
* [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
* 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
* 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: 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: [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: [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: 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: 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: 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: 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: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
* [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/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
* 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: [net v6 1/8] i40e: main driver core
From: Francois Romieu @ 2013-09-07 23:01 UTC (permalink / raw)
To: Brandeburg, Jesse
Cc: Daniel Borkmann, 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: <1378581636.2509.12.camel@jbrandeb-mobl.amr.corp.intel.com>
Brandeburg, Jesse <jesse.brandeburg@intel.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.
It should always be made. You have kept ignoring it since june (see
Ben Hutchings's comment on 2013/06/19).
Where did you see that rules changed and the linux kernel should care
about OS shared code ?
- the patchkit does not include a Makefile to compile from patch #1
- it would not compile since patch #1 depends on stuff that appears
later (see i40e_hw or i40e_lump_tracking for instance).
- some of the I40E_ERR_PARAM error can't happen or are assert() in
disguise that could / should instead BUG(). There is no reason to
confuse these with runtime failures, especially as code gets really
tested on hardware.
I could understand it from some newly introduced company but it's
a bit deceptive from Intel. :o/
--
Ueimor
^ permalink raw reply
* [PATCH net 1/1] r8169: enforce RX_MULTI_EN for the 8168f.
From: Francois Romieu @ 2013-09-07 23:15 UTC (permalink / raw)
To: netdev; +Cc: David Miller, David R, Frédéric Leroy, Hayes Wang
Same narrative as eb2dc35d99028b698cdedba4f5522bc43e576bd2 ("r8169: RxConfig
hack for the 8168evl.") regarding AMD IOMMU errors.
RTL_GIGA_MAC_VER_36 - 8168f as well - has not been reported to behave the
same.
Tested-by: David R <david@unsolicited.net>
Tested-by: Frédéric Leroy <fredo@starox.org>
Cc: Hayes Wang <hayeswang@realtek.com>
---
Hayes, a ack would be welcome.
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:
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] bnx2x: avoid atomic allocations during initialization
From: David Miller @ 2013-09-08 1:48 UTC (permalink / raw)
To: dkravkov; +Cc: dmitry, mschmidt, netdev, ariele, eilong
In-Reply-To: <CAM8tLiPCUS3_jo4KU1zY9+8wRg08fA0ohH9d+0a4jyC69D9JZg@mail.gmail.com>
From: Dmitry Kravkov <dkravkov@gmail.com>
Date: Sat, 7 Sep 2013 12:07:50 +0300
> On Sat, Sep 7, 2013 at 7:29 AM, David Miller <davem@davemloft.net> wrote:
>> Probe should never fail because of an atomic memory allocation if at
>> all possible.
> We here are in open() flow, not probe()
It's the same from my perspective.
Better to drop some packets than have _COMPLETELY INOPERATIVE INTERFACE_.
^ permalink raw reply
* Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: David Miller @ 2013-09-08 1:50 UTC (permalink / raw)
To: eldad; +Cc: jiri, netdev, kuznet, jmorris, yoshfuji, kaber
In-Reply-To: <alpine.LNX.2.00.1309071427120.1262@heraclitus>
From: Eldad Zack <eldad@fogrefinery.com>
Date: Sat, 7 Sep 2013 14:31:36 +0200 (CEST)
> On Fri, 6 Sep 2013, Jiri Pirko wrote:
>
>> Current behaviour breaks TAHI Test v6LC.1.2.6.
>
> I'm not familiar with this, but IMHO the test should be reversed :)
I think I agree with Eldad on this, and that TAHI should be adjusted
to have more reasonable expectations of an implementation.
^ permalink raw reply
* [010/121] sctp: fully initialize sctp_outq in sctp_outq_init
From: Ben Hutchings @ 2013-09-08 2:52 UTC (permalink / raw)
To: linux-kernel, stable
Cc: akpm, davem, Vlad Yasevich, Neil Horman, netdev,
West, Steve (NSN - US/Fort Worth)
In-Reply-To: <lsq.1378608720.401499712@decadent.org.uk>
3.2.51-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Neil Horman <nhorman@tuxdriver.com>
[ Upstream commit c5c7774d7eb4397891edca9ebdf750ba90977a69 ]
In commit 2f94aabd9f6c925d77aecb3ff020f1cc12ed8f86
(refactor sctp_outq_teardown to insure proper re-initalization)
we modified sctp_outq_teardown to use sctp_outq_init to fully re-initalize the
outq structure. Steve West recently asked me why I removed the q->error = 0
initalization from sctp_outq_teardown. I did so because I was operating under
the impression that sctp_outq_init would properly initalize that value for us,
but it doesn't. sctp_outq_init operates under the assumption that the outq
struct is all 0's (as it is when called from sctp_association_init), but using
it in __sctp_outq_teardown violates that assumption. We should do a memset in
sctp_outq_init to ensure that the entire structure is in a known state there
instead.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: "West, Steve (NSN - US/Fort Worth)" <steve.west@nsn.com>
CC: Vlad Yasevich <vyasevich@gmail.com>
CC: netdev@vger.kernel.org
CC: davem@davemloft.net
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
net/sctp/outqueue.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 96eb168..3dd7207 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -205,6 +205,8 @@ static inline int sctp_cacc_skip(struct sctp_transport *primary,
*/
void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q)
{
+ memset(q, 0, sizeof(struct sctp_outq));
+
q->asoc = asoc;
INIT_LIST_HEAD(&q->out_chunk_list);
INIT_LIST_HEAD(&q->control_chunk_list);
@@ -212,13 +214,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q)
INIT_LIST_HEAD(&q->sacked);
INIT_LIST_HEAD(&q->abandoned);
- q->fast_rtx = 0;
- q->outstanding_bytes = 0;
q->empty = 1;
- q->cork = 0;
-
- q->malloced = 0;
- q->out_qlen = 0;
}
/* Free the outqueue structure and any related pending chunks.
^ permalink raw reply related
* Re: [PATCH] net: fix multiqueue selection
From: Alexander Duyck @ 2013-09-08 4:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Alexander Duyck
In-Reply-To: <1378580577.26319.26.camel@edumazet-glaptop>
On 09/07/2013 12:02 PM, Eric Dumazet wrote:
> 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;
> }
>
>
> --
Ugh, my bad. This is a nasty one too since the behaviour appeared to be
correct for most cases.
It looks like this needs to go into stable for 3.9 - 3.11.
Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
^ permalink raw reply
* Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: YOSHIFUJI Hideaki @ 2013-09-08 4:20 UTC (permalink / raw)
To: David Miller
Cc: eldad, jiri, netdev, kuznet, jmorris, kaber, YOSHIFUJI Hideaki
In-Reply-To: <20130907.215020.38125291010941900.davem@davemloft.net>
Hi.
David Miller wrote:
> From: Eldad Zack <eldad@fogrefinery.com>
> Date: Sat, 7 Sep 2013 14:31:36 +0200 (CEST)
>
>> On Fri, 6 Sep 2013, Jiri Pirko wrote:
>>
>>> Current behaviour breaks TAHI Test v6LC.1.2.6.
>>
>> I'm not familiar with this, but IMHO the test should be reversed :)
>
> I think I agree with Eldad on this, and that TAHI should be adjusted
> to have more reasonable expectations of an implementation.
>
The tests (including packet formats) are based on the test specification:
https://www.ipv6ready.org/?page=documents&tag=ipv6-core-protocols
If we want to change the tests, I think we need to consult with IPv6
Forum.
Regards,
--yoshfuji
^ permalink raw reply
* Re: [PATCH net] net: ovs: flow: fix potential illegal memory access in __parse_flow_nlattrs
From: Jesse Gross @ 2013-09-08 5:35 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David Miller, netdev, Andy Zhou, dev@openvswitch.org
In-Reply-To: <1378539694-3635-1-git-send-email-dborkman@redhat.com>
On Sat, Sep 7, 2013 at 12:41 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> In function __parse_flow_nlattrs(), we check for condition
> (type > OVS_KEY_ATTR_MAX) and if true, print an error, but we do
> not return from this function as in other checks. It seems this
> has been forgotten, as otherwise, we could access beyond the
> memory of ovs_key_lens, which is of ovs_key_lens[OVS_KEY_ATTR_MAX + 1].
> Hence, a maliciously prepared nla_type from user space could access
> beyond this upper limit.
>
> Introduced by 03f0d916a ("openvswitch: Mega flow implementation").
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Andy Zhou <azhou@nicira.com>
Yeah, looks like a mistake to me.
Acked-by: Jesse Gross <jesse@nicira.com>
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Ding Tianhong @ 2013-09-08 6:05 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Nikolay Aleksandrov, Ding Tianhong, Jay Vosburgh, Andy Gospodarek,
David S. Miller, Netdev
In-Reply-To: <20130907150350.GF26163@redhat.com>
于 2013/9/7 23:03, Veaceslav Falico 写道:
> 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.
>
Although RCU is truely difficult to understand, but it is very
interesting and beautiful,
I will follow your footsteps and finish it.
>>
>> But in any case I complete agree with Veaceslav here. Read all the
>> documentation carefully :-)
>>
>> Cheers,
>> Nik
>>
good lession, read it complete and agreed, it is a hard work to do.
I need more time to review the Veaceslav's patchset and improve my
thought, thanks Nik and Veaceslav.
Best regards
Ding
>>> /**
>>> * 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.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net 1/1] r8169: enforce RX_MULTI_EN for the 8168f.
From: Daniel Borkmann @ 2013-09-08 8:45 UTC (permalink / raw)
To: Francois Romieu
Cc: netdev, David Miller, David R, Frédéric Leroy,
Hayes Wang
In-Reply-To: <20130907231535.GB24530@electric-eye.fr.zoreil.com>
On 09/08/2013 01:15 AM, Francois Romieu wrote:
> Same narrative as eb2dc35d99028b698cdedba4f5522bc43e576bd2 ("r8169: RxConfig
> hack for the 8168evl.") regarding AMD IOMMU errors.
>
> RTL_GIGA_MAC_VER_36 - 8168f as well - has not been reported to behave the
> same.
>
> Tested-by: David R <david@unsolicited.net>
> Tested-by: Frédéric Leroy <fredo@starox.org>
> Cc: Hayes Wang <hayeswang@realtek.com>
> ---
Your signed-off-by is missing.
^ 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