* Re: [patch net-next v3 17/17] rocker: add ndo_bridge_setlnk/getlink support for learning policy
From: Jiri Pirko @ 2014-11-26 11:27 UTC (permalink / raw)
To: Thomas Graf
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, f.fainelli, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <20141126110709.GC4321@casper.infradead.org>
Wed, Nov 26, 2014 at 12:07:09PM CET, tgraf@suug.ch wrote:
>On 11/25/14 at 11:28am, Jiri Pirko wrote:
>> @@ -3657,6 +3693,64 @@ skip:
>> return idx;
>> }
>>
>> +static int rocker_port_bridge_setlink(struct net_device *dev,
>> + struct nlmsghdr *nlh)
>> +{
>> + struct rocker_port *rocker_port = netdev_priv(dev);
>> + struct nlattr *protinfo;
>> + struct nlattr *afspec;
>> + struct nlattr *attr;
>> + u16 mode;
>> + int err;
>> +
>> + protinfo = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
>> + IFLA_PROTINFO);
>> + afspec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
>> +
>> + if (afspec) {
>> + attr = nla_find_nested(afspec, IFLA_BRIDGE_MODE);
>> + if (attr) {
>> + mode = nla_get_u16(attr);
>> + if (mode != BRIDGE_MODE_SWDEV)
>> + return -EINVAL;
>> + }
>> + }
>
>The Netlink message is completely unverified at this point. All
>rtnl_bridge_setlink() does is verify that msgsize >= ifinfomsg.
>All of the drivers but br_setlink() need fixing in this regard.
I believe that we should fix this for all drivers in a follow-up patch.
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Jamal Hadi Salim @ 2014-11-26 11:28 UTC (permalink / raw)
To: Scott Feldman
Cc: John Fastabend, Jiri Pirko, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, Thomas Graf,
dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com,
pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk,
stephen@networkplumber.org, Kirsher, Jeffrey T,
vyasevic@redhat.com, Cong Wang, Eric Dumazet, Florian Fainelli,
Roopa Prabhu, John Linville
In-Reply-To: <CAE4R7bA2VPZ3tm2MrsXAm4XfNX0TguT=vUma-3HrZSu98hBGyA@mail.gmail.com>
On 11/25/14 22:59, Scott Feldman wrote:
> On Tue, Nov 25, 2014 at 5:19 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 11/25/14 21:36, Scott Feldman wrote:
>>
>>>> Ok, guess i am gonna have to go stare at the code some more.
>>>> I thought we returned one of the error codes?
>>>> A bitmask would work for a single entry - because you have two
>>>> options add to h/ware and/or s/ware. So response is easy to encode.
>>>> But if i have 1000 and they are sparsely populated (think an indexed
>>>> table and i have indices 1, 23, 45, etc), then a bitmask would be
>>>> hard to use.
>>>
>>>
>>> I'm confused by this discussion.
>>
>>
>> This is about the policy which states "install as many as you can, dont
>> worry about failures". In such a case, how do you tell user space back
>> "oh, btw you know your request #1, #23, and 45 went ok, but nothing else
>> worked". A simple return code wont work. You could return a code to
>> say "some worked". At which case user space could dump and find out only
>> #1, #23 and #45 worked.
>
> You request for what? That's my confusion.
Scott, you are gonna make do this all over again?;->
The summary is there are three possible policies that could be
identified by the user asking for a kernel operation.
One use case example was to send a bunch of (for example)
create/updates and request that the kernel should not abort on a
failure of a single one but to keep going and create/update as many
as possible. Is that part clear? I know it is not what you do,
but there are use cases for that (Read John's response).
Now assuming someone wants this and some entries failed;
how do you tell user space back what was actually updated vs not?
You could return a code which says "partial success".
Forget whether the table is keyed or indexed but if you wanted
to return more detailed info you would return an array/vector of some
sort with status code per entry. Something netlink cant do.
Is that a better description?
> Are you trying to install
> FDB entry into both SW and HW at same time?
What is wrong with installing on both hardware and software? The
point was to identify what kind of policies could be requested by
the user; but even for the bridge why is it bad that i ask for
both master&self?
It is something I can do today with none of these patches.
> And then do a bunch in a
> batch? I'm saying use MASTER for SW and SELF for HW in two steps,
But that would be enforcing your policy on me.
> if
> you want FDB entry installed in both Sw and HW. Check your return
> code each step. Batch all to HW first, then batch all that PASSED to
> SW. I don't even know really why you're trying to install to both HW
> and SW. Install it to HW and be done. fdb_dump will set HW entries
> via SELF.
>
First off: bad performance, but your call to do it that way
(just please please dont enforce it on me;->)
Lets take the hardware batching you mentioned above and see if
i can help to clarify in the third policy choice (continue-on-failure).
Lets say you have a keyed table such as the fdb table is.
You send 10 entries to be created/added in hardware. #3 and #5 failed
because you made a mistake and sent them with the same key. #9 and #10
failed because the hardware doesnt have any more space.
we didnt stop and go back for #3 and #5 because the user told
us to continue and do the rest when we fail. And s/he did that because
she wanted to put as many entries in hardware as possible without
necessarily needing to know how much space exists.
> Ah, Jamal, look again at patches 13-17/17 in last v3 set. That was a
> big steaming snickerdoodle just for you! Now you can push policy
> knobs down to port driver and or bridge to fine tune what ever you
> want. You'll find knobs for learning, flooding, learning sync to hw,
> etc. I thought you even ACKed some of these.
I think it almost there.
What you are missing is the policy decision to only sync when i
say so. Having an ndo_ops is a necessity but i dont want the driver
to decide for me just because it can ;->
Telling hardware to learn is instructing it to self update its entries
based on source lookup failure. That is distinctly different from
telling to sync to the kernel. So if you add that knob we are in good
shape.
cheers,
jamal
> a) above knob is 14/17
> patch, b) above is using existing learning knob on bridge, c) above I
> don't get...no point in syncing that direction.
>
^ permalink raw reply
* Re: [patch net-next v3 17/17] rocker: add ndo_bridge_setlnk/getlink support for learning policy
From: Thomas Graf @ 2014-11-26 11:30 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, f.fainelli, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <20141126112718.GK1875@nanopsycho.orion>
On 11/26/14 at 12:27pm, Jiri Pirko wrote:
> Wed, Nov 26, 2014 at 12:07:09PM CET, tgraf@suug.ch wrote:
> >The Netlink message is completely unverified at this point. All
> >rtnl_bridge_setlink() does is verify that msgsize >= ifinfomsg.
> >All of the drivers but br_setlink() need fixing in this regard.
>
> I believe that we should fix this for all drivers in a follow-up patch.
I'm working on this. Will send fixes later today.
^ permalink raw reply
* Re: [PATCH v2] net: netfilter: Fix undefined reference to nf_nat_redirect_* functions
From: Florian Westphal @ 2014-11-26 11:33 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Andreas Ruprecht, Florian Westphal, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, netfilter-devel, coreteam,
netdev, linux-kernel
In-Reply-To: <20141126112449.GA4700@salvia>
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 2) Merge nf_nat_redirect_ipv4 and nf_nat_redirect_ipv6 into
> nf_nat_redirect, so we inconditionally build IPv6 redirect code, thus
> xt_REDIRECT always finds the IPv6 symbol that needs even if it doesn't
> use it.
Seems this is the best solution since it would also reduce the
kconfig option symbol count.
afaics there is nothing ipv6 specific in nf_nat_redirect_ipv6 so
this should not cause any of the ususal IPV6=m dependency issues.
^ permalink raw reply
* Re: [patch net-next v3 04/17] net: introduce generic switch devices support
From: Jamal Hadi Salim @ 2014-11-26 11:36 UTC (permalink / raw)
To: Scott Feldman
Cc: Thomas Graf, Jiri Pirko, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville
In-Reply-To: <CAE4R7bCYwWt5cem5_v8Y0jOSz5ER5_33bMRE2xkTPOqYvM6dUg@mail.gmail.com>
On 11/25/14 23:18, Scott Feldman wrote:
> On Tue, Nov 25, 2014 at 5:33 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> You have a pointer to the kernel driver for that HW?
I wasnt sure if that was a passive aggressive move there to
question what i am claiming?(Only Canadians are allowed to be
passive aggressive Scott). To answer your question, no
code currently littered with vendor SDK unfortunately (as you
would know!).
But hopefully if we get these changes in correctly it would
not be hard to show the driver working fully in the kernel.
There are definetely a few other pieces of hardware that are
making me come back here and invest time and effort in these
long discussions.
> Can you show how
> you're using Linux tc netlink msg in kernel to program HW? I'd like
> to see the in-kernel API.
>
Lets do the L2/port thing first. But yes, I am using Linux tc in
kernel.
cheers,
jamal
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Jiri Pirko @ 2014-11-26 11:40 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Scott Feldman, John Fastabend, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, Thomas Graf,
dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com,
pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk,
stephen@networkplumber.org, Kirsher, Jeffrey T,
vyasevic@redhat.com, Cong Wang, Eric Dumazet, Florian Fainelli,
Roopa Prabhu, John Linville
In-Reply-To: <5475B952.2080500@mojatatu.com>
Wed, Nov 26, 2014 at 12:28:18PM CET, jhs@mojatatu.com wrote:
>On 11/25/14 22:59, Scott Feldman wrote:
>>On Tue, Nov 25, 2014 at 5:19 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>On 11/25/14 21:36, Scott Feldman wrote:
>
>>>
>>>>>Ok, guess i am gonna have to go stare at the code some more.
>>>>>I thought we returned one of the error codes?
>>>>>A bitmask would work for a single entry - because you have two
>>>>>options add to h/ware and/or s/ware. So response is easy to encode.
>>>>>But if i have 1000 and they are sparsely populated (think an indexed
>>>>>table and i have indices 1, 23, 45, etc), then a bitmask would be
>>>>>hard to use.
>>>>
>>>>
>>>>I'm confused by this discussion.
>>>
>>>
>>>This is about the policy which states "install as many as you can, dont
>>>worry about failures". In such a case, how do you tell user space back
>>>"oh, btw you know your request #1, #23, and 45 went ok, but nothing else
>>>worked". A simple return code wont work. You could return a code to
>>>say "some worked". At which case user space could dump and find out only
>>>#1, #23 and #45 worked.
>>
>>You request for what? That's my confusion.
>
>Scott, you are gonna make do this all over again?;->
>The summary is there are three possible policies that could be
>identified by the user asking for a kernel operation.
>One use case example was to send a bunch of (for example)
>create/updates and request that the kernel should not abort on a
>failure of a single one but to keep going and create/update as many
>as possible. Is that part clear? I know it is not what you do,
>but there are use cases for that (Read John's response).
>Now assuming someone wants this and some entries failed;
>how do you tell user space back what was actually updated vs not?
>You could return a code which says "partial success".
>Forget whether the table is keyed or indexed but if you wanted
>to return more detailed info you would return an array/vector of some
>sort with status code per entry. Something netlink cant do.
>Is that a better description?
Sure this is something that is reasonable to request. But that would
require a major changes to userspace api. At this moment, when we are
using the existing api, I would leave this out for phase 1. Let this be
resolved later as a separate work. Does that make sense?
>
>>Are you trying to install
>>FDB entry into both SW and HW at same time?
>
>
>What is wrong with installing on both hardware and software? The
>point was to identify what kind of policies could be requested by
>the user; but even for the bridge why is it bad that i ask for
>both master&self?
>It is something I can do today with none of these patches.
>
>>And then do a bunch in a
>>batch? I'm saying use MASTER for SW and SELF for HW in two steps,
>
>But that would be enforcing your policy on me.
>
>>if
>>you want FDB entry installed in both Sw and HW. Check your return
>>code each step. Batch all to HW first, then batch all that PASSED to
>>SW. I don't even know really why you're trying to install to both HW
>>and SW. Install it to HW and be done. fdb_dump will set HW entries
>>via SELF.
>>
>
>First off: bad performance, but your call to do it that way
>(just please please dont enforce it on me;->)
>
>Lets take the hardware batching you mentioned above and see if
>i can help to clarify in the third policy choice (continue-on-failure).
>Lets say you have a keyed table such as the fdb table is.
>You send 10 entries to be created/added in hardware. #3 and #5 failed
>because you made a mistake and sent them with the same key. #9 and #10
>failed because the hardware doesnt have any more space.
>we didnt stop and go back for #3 and #5 because the user told
>us to continue and do the rest when we fail. And s/he did that because
>she wanted to put as many entries in hardware as possible without
>necessarily needing to know how much space exists.
>
>
>>Ah, Jamal, look again at patches 13-17/17 in last v3 set. That was a
>>big steaming snickerdoodle just for you! Now you can push policy
>>knobs down to port driver and or bridge to fine tune what ever you
>>want. You'll find knobs for learning, flooding, learning sync to hw,
>>etc. I thought you even ACKed some of these.
>
>I think it almost there.
>What you are missing is the policy decision to only sync when i
>say so. Having an ndo_ops is a necessity but i dont want the driver
>to decide for me just because it can ;->
>Telling hardware to learn is instructing it to self update its entries
>based on source lookup failure. That is distinctly different from
>telling to sync to the kernel. So if you add that knob we are in good
>shape.
>
>cheers,
>jamal
>
>>a) above knob is 14/17
>>patch, b) above is using existing learning knob on bridge, c) above I
>>don't get...no point in syncing that direction.
>>
>
^ permalink raw reply
* Re: [patch net-next v3 17/17] rocker: add ndo_bridge_setlnk/getlink support for learning policy
From: Jiri Pirko @ 2014-11-26 11:42 UTC (permalink / raw)
To: Thomas Graf
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, f.fainelli, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <20141126113024.GA8410@casper.infradead.org>
Wed, Nov 26, 2014 at 12:30:24PM CET, tgraf@suug.ch wrote:
>On 11/26/14 at 12:27pm, Jiri Pirko wrote:
>> Wed, Nov 26, 2014 at 12:07:09PM CET, tgraf@suug.ch wrote:
>> >The Netlink message is completely unverified at this point. All
>> >rtnl_bridge_setlink() does is verify that msgsize >= ifinfomsg.
>> >All of the drivers but br_setlink() need fixing in this regard.
>>
>> I believe that we should fix this for all drivers in a follow-up patch.
>
>I'm working on this. Will send fixes later today.
Feel free to fix rocker as well. I'll take your patch into my queue.
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Jamal Hadi Salim @ 2014-11-26 11:54 UTC (permalink / raw)
To: Jiri Pirko
Cc: Scott Feldman, John Fastabend, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, Thomas Graf,
dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com,
pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk,
stephen@networkplumber.org, Kirsher, Jeffrey T,
vyasevic@redhat.com, Cong Wang, Eric Dumazet, Florian Fainelli,
Roopa Prabhu, John Linville
In-Reply-To: <20141126114035.GM1875@nanopsycho.orion>
On 11/26/14 06:40, Jiri Pirko wrote:
> Wed, Nov 26, 2014 at 12:28:18PM CET, jhs@mojatatu.com wrote:
>> Scott, you are gonna make do this all over again?;->
>> The summary is there are three possible policies that could be
>> identified by the user asking for a kernel operation.
>> One use case example was to send a bunch of (for example)
>> create/updates and request that the kernel should not abort on a
>> failure of a single one but to keep going and create/update as many
>> as possible. Is that part clear? I know it is not what you do,
>> but there are use cases for that (Read John's response).
>> Now assuming someone wants this and some entries failed;
>> how do you tell user space back what was actually updated vs not?
>> You could return a code which says "partial success".
>> Forget whether the table is keyed or indexed but if you wanted
>> to return more detailed info you would return an array/vector of some
>> sort with status code per entry. Something netlink cant do.
>> Is that a better description?
>
> Sure this is something that is reasonable to request. But that would
> require a major changes to userspace api. At this moment, when we are
> using the existing api, I would leave this out for phase 1. Let this be
> resolved later as a separate work. Does that make sense?
>
I think these are just discussions so we know where we are going.
I ACKed the patch already but added that we should consider these
policies. Scott take note.
The default behavior should be maintained whatever the new policies are.
The vectoring is going to be a harder thing to get right. It can be done
but long shot probably.
For user->kernel policy description, that is easy; we need 2 bits
from somewhere; probably same namespace as software/hardware.
cheers,
jamal
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Jamal Hadi Salim @ 2014-11-26 12:06 UTC (permalink / raw)
To: Jiri Pirko
Cc: Scott Feldman, John Fastabend, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, Thomas Graf,
dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com,
pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk,
stephen@networkplumber.org, Kirsher, Jeffrey T,
vyasevic@redhat.com, Cong Wang, Eric Dumazet, Florian Fainelli,
Roopa Prabhu, John Linville
In-Reply-To: <5475BF72.1060606@mojatatu.com>
On 11/26/14 06:54, Jamal Hadi Salim wrote:
> On 11/26/14 06:40, Jiri Pirko wrote:
>> Sure this is something that is reasonable to request. But that would
>> require a major changes to userspace api. At this moment, when we are
>> using the existing api, I would leave this out for phase 1. Let this be
>> resolved later as a separate work. Does that make sense?
>>
>
> I think these are just discussions so we know where we are going.
> I ACKed the patch already but added that we should consider these
> policies. Scott take note.
>
In case i wasnt clear - yes, the patch as is fine ;->
cheers,
jamal
^ permalink raw reply
* Re: [patch net-next v3 13/17] bridge: move private brport flags to if_bridge.h so port drivers can use flags
From: Jiri Pirko @ 2014-11-26 12:12 UTC (permalink / raw)
To: Thomas Graf
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, f.fainelli, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <20141125224856.GI3912@casper.infradead.org>
Tue, Nov 25, 2014 at 11:48:56PM CET, tgraf@suug.ch wrote:
>On 11/25/14 at 11:28am, Jiri Pirko wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
>Could be ported to BIT() if you respin.
Done, thanks.
^ permalink raw reply
* Re: [PATCH v4 0/7] kernel tinification: optionally compile out splice family of syscalls (splice, vmsplice, tee and sendfile)
From: One Thousand Gnomes @ 2014-11-26 12:19 UTC (permalink / raw)
To: David Miller
Cc: josh, rdunlap, pieter, alexander.h.duyck, viro, ast, akpm, beber,
catalina.mocanu, dborkman, edumazet, ebiederm, fabf, fuse-devel,
geert, hughd, iulia.manda21, JBeulich, bfields, jlayton,
linux-api, linux-fsdevel, linux-kernel, linux-nfs, mcgrof,
mattst88, mgorman, mst, miklos, netdev, oleg, Paul.Durrant,
paulmck, pefoley2, tgraf, therbert, trond.myklebust, willemb,
xiaoguangrong
In-Reply-To: <20141125.140441.401150380839514113.davem@davemloft.net>
On Tue, 25 Nov 2014 14:04:41 -0500 (EST)
David Miller <davem@davemloft.net> wrote:
> From: josh@joshtriplett.org
> Date: Tue, 25 Nov 2014 10:53:10 -0800
>
> > It's not a "slippery slope"; it's been our standard practice for ages.
>
> We've never put an entire class of generic system calls behind
> a config option.
Try running an original MCC Linux binary and C lib on a current kernel
We've put *entire binary formats* behind a config option. We've put older
syscalls behind it, we've put sysfs behind it, sysctl behind it, the
older microcode interfaces behind it, ISA bus as a concept behind
options. VDSO, IPC, even 32bit support ... the list goes on and on.
I'd say those were far more generic on the whole than splice/sendfile.
Alan
^ permalink raw reply
* Re: [PATCH v4 20/42] virtio_net: pass vi around
From: Cornelia Huck @ 2014-11-26 12:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <1416933600-21398-21-git-send-email-mst@redhat.com>
On Tue, 25 Nov 2014 18:42:49 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Too many places poke at [rs]q->vq->vdev->priv just to get
> the the vi structure. Let's just pass the pointer around: seems
s/the the/the/
> cleaner, and might even be faster.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 38 ++++++++++++++++++++------------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
Looks reasonable.
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* [PATCH 0/5 net] bridge: Fix missing Netlink message validations
From: Thomas Graf @ 2014-11-26 12:42 UTC (permalink / raw)
To: davem; +Cc: stephen, netdev
Adds various missing length checks in the bridging code for Netlink
messages and corresponding attributes provided by user space.
Thomas Graf (5):
bridge: Validate IFLA_BRIDGE_FLAGS attribute length
net: Validate IFLA_BRIDGE_MODE attribute length
net: Check for presence of IFLA_AF_SPEC
bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
net/bridge/br_netlink.c | 1 +
net/core/rtnetlink.c | 23 ++++++++++++++++++-----
4 files changed, 29 insertions(+), 5 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH 1/5] bridge: Validate IFLA_BRIDGE_FLAGS attribute length
From: Thomas Graf @ 2014-11-26 12:42 UTC (permalink / raw)
To: davem; +Cc: stephen, netdev, Vlad Yasevich
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
Payload is currently accessed blindly and may exceed valid message
boundaries.
Fixes: 407af3299 ("bridge: Add netlink interface to configure vlans on bridge ports")
Cc: Vlad Yasevich <vyasevic@redhat.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
net/core/rtnetlink.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a688268..5a853f8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2798,6 +2798,9 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh)
if (br_spec) {
nla_for_each_nested(attr, br_spec, rem) {
if (nla_type(attr) == IFLA_BRIDGE_FLAGS) {
+ if (nla_len(attr) < sizeof(flags))
+ return -EINVAL;
+
have_flags = true;
flags = nla_get_u16(attr);
break;
@@ -2868,6 +2871,9 @@ static int rtnl_bridge_dellink(struct sk_buff *skb, struct nlmsghdr *nlh)
if (br_spec) {
nla_for_each_nested(attr, br_spec, rem) {
if (nla_type(attr) == IFLA_BRIDGE_FLAGS) {
+ if (nla_len(attr) < sizeof(flags))
+ return -EINVAL;
+
have_flags = true;
flags = nla_get_u16(attr);
break;
--
1.9.3
^ permalink raw reply related
* [PATCH 2/5] net: Validate IFLA_BRIDGE_MODE attribute length
From: Thomas Graf @ 2014-11-26 12:42 UTC (permalink / raw)
To: davem; +Cc: stephen, netdev, Ajit Khaparde, John Fastabend
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
Payload is currently accessed blindly and may exceed valid message
boundaries.
Fixes: a77dcb8c8 ("be2net: set and query VEB/VEPA mode of the PF interface")
Fixes: 815cccbf1 ("ixgbe: add setlink, getlink support to ixgbe and ixgbevf")
Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
drivers/net/ethernet/emulex/benet/be_main.c | 3 +++
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 3e8475c..337e4cd 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4314,6 +4314,9 @@ static int be_ndo_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh)
if (nla_type(attr) != IFLA_BRIDGE_MODE)
continue;
+ if (nla_len(attr) < sizeof(mode))
+ return -EINVAL;
+
mode = nla_get_u16(attr);
if (mode != BRIDGE_MODE_VEPA && mode != BRIDGE_MODE_VEB)
return -EINVAL;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 82ffe8b..dff9905 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7677,6 +7677,9 @@ static int ixgbe_ndo_bridge_setlink(struct net_device *dev,
if (nla_type(attr) != IFLA_BRIDGE_MODE)
continue;
+ if (nla_len(attr) < sizeof(mode))
+ return -EINVAL;
+
mode = nla_get_u16(attr);
if (mode == BRIDGE_MODE_VEPA) {
reg = 0;
--
1.9.3
^ permalink raw reply related
* [PATCH 3/5] net: Check for presence of IFLA_AF_SPEC
From: Thomas Graf @ 2014-11-26 12:42 UTC (permalink / raw)
To: davem; +Cc: stephen, netdev, Ajit Khaparde, John Fastabend
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
ndo_bridge_setlink() is currently only called on the slave if
IFLA_AF_SPEC is set but this is a very fragile assumption and may
change in the future.
Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
drivers/net/ethernet/emulex/benet/be_main.c | 2 ++
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 337e4cd..597c463 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4309,6 +4309,8 @@ static int be_ndo_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh)
return -EOPNOTSUPP;
br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
+ if (!br_spec)
+ return -EINVAL;
nla_for_each_nested(attr, br_spec, rem) {
if (nla_type(attr) != IFLA_BRIDGE_MODE)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index dff9905..cc51554 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7669,6 +7669,8 @@ static int ixgbe_ndo_bridge_setlink(struct net_device *dev,
return -EOPNOTSUPP;
br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
+ if (!br_spec)
+ return -EINVAL;
nla_for_each_nested(attr, br_spec, rem) {
__u16 mode;
--
1.9.3
^ permalink raw reply related
* [PATCH 4/5] bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
From: Thomas Graf @ 2014-11-26 12:42 UTC (permalink / raw)
To: davem; +Cc: stephen, netdev
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
Fixes: c2d3babf ("bridge: implement multicast fast leave")
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
net/bridge/br_netlink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 2ff9706..e5ec470 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -280,6 +280,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
[IFLA_BRPORT_MODE] = { .type = NLA_U8 },
[IFLA_BRPORT_GUARD] = { .type = NLA_U8 },
[IFLA_BRPORT_PROTECT] = { .type = NLA_U8 },
+ [IFLA_BRPORT_FAST_LEAVE]= { .type = NLA_U8 },
[IFLA_BRPORT_LEARNING] = { .type = NLA_U8 },
[IFLA_BRPORT_UNICAST_FLOOD] = { .type = NLA_U8 },
};
--
1.9.3
^ permalink raw reply related
* [PATCH 5/5] bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
From: Thomas Graf @ 2014-11-26 12:42 UTC (permalink / raw)
To: davem; +Cc: stephen, netdev, Vlad Yasevich
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
Only search for IFLA_EXT_MASK if the message actually carries a
ifinfomsg header and validate minimal length requirements for
IFLA_EXT_MASK.
Fixes: 6cbdceeb ("bridge: Dump vlan information from a bridge port")
Cc: Vlad Yasevich <vyasevic@redhat.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
net/core/rtnetlink.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5a853f8..b9b7dfa 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2685,13 +2685,20 @@ static int rtnl_bridge_getlink(struct sk_buff *skb, struct netlink_callback *cb)
int idx = 0;
u32 portid = NETLINK_CB(cb->skb).portid;
u32 seq = cb->nlh->nlmsg_seq;
- struct nlattr *extfilt;
u32 filter_mask = 0;
- extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg),
- IFLA_EXT_MASK);
- if (extfilt)
- filter_mask = nla_get_u32(extfilt);
+ if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
+ struct nlattr *extfilt;
+
+ extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg),
+ IFLA_EXT_MASK);
+ if (extfilt) {
+ if (nla_len(extfilt) < sizeof(filter_mask))
+ return -EINVAL;
+
+ filter_mask = nla_get_u32(extfilt);
+ }
+ }
rcu_read_lock();
for_each_netdev_rcu(net, dev) {
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v4 21/42] virtio_net: get rid of virtio_net_hdr/skb_vnet_hdr
From: Cornelia Huck @ 2014-11-26 12:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <1416933600-21398-22-git-send-email-mst@redhat.com>
On Tue, 25 Nov 2014 18:42:53 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> virtio 1.0 doesn't use virtio_net_hdr anymore, and in fact, it's not
> really useful since virtio_net_hdr_mrg_rxbuf includes that as the first
> field anyway.
>
> Let's drop it, precalculate header len and store within vi instead.
>
> This way we can also remove struct skb_vnet_hdr.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 90 ++++++++++++++++++++++--------------------------
> 1 file changed, 41 insertions(+), 49 deletions(-)
>
> @@ -260,13 +256,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>
> hdr = skb_vnet_hdr(skb);
>
> - if (vi->mergeable_rx_bufs) {
> - hdr_len = sizeof hdr->mhdr;
> - hdr_padded_len = sizeof hdr->mhdr;
> - } else {
> - hdr_len = sizeof hdr->hdr;
> + hdr_len = vi->hdr_len;
> + if (vi->mergeable_rx_bufs)
> + hdr_padded_len = sizeof *hdr;
sizeof without () looks weird to me.
> + else
> hdr_padded_len = sizeof(struct padded_vnet_hdr);
> - }
>
> memcpy(hdr, p, hdr_len);
>
Otherwise:
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v4 22/42] virtio_net: stricter short buffer length checks
From: Cornelia Huck @ 2014-11-26 13:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <1416933600-21398-23-git-send-email-mst@redhat.com>
On Tue, 25 Nov 2014 18:42:58 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Our buffer length check is not strict enough for mergeable
> buffers: buffer can still be shorter that header + address
> by 2 bytes.
>
> Fix that up.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v4 23/42] virtio_net: bigger header when VERSION_1 is set
From: Cornelia Huck @ 2014-11-26 13:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <1416933600-21398-24-git-send-email-mst@redhat.com>
On Tue, 25 Nov 2014 18:43:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> With VERSION_1 virtio_net uses same header size
> whether mergeable buffers are enabled or not.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v4 24/42] virtio_net: enable v1.0 support
From: Cornelia Huck @ 2014-11-26 13:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <1416933600-21398-25-git-send-email-mst@redhat.com>
On Tue, 25 Nov 2014 18:43:06 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Now that we have completed 1.0 support, enable it in our driver.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 1 +
> 1 file changed, 1 insertion(+)
>
Hm. The spec states that mac is driver-writable in the legacy case.
Don't we need to fence writing it in virtnet_set_mac_address() in the
virtio 1.0 case?
^ permalink raw reply
* Re: [PATCH v4 21/42] virtio_net: get rid of virtio_net_hdr/skb_vnet_hdr
From: Michael S. Tsirkin @ 2014-11-26 13:11 UTC (permalink / raw)
To: Cornelia Huck
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126135053.1bf4f97c.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 01:50:53PM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 18:42:53 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > virtio 1.0 doesn't use virtio_net_hdr anymore, and in fact, it's not
> > really useful since virtio_net_hdr_mrg_rxbuf includes that as the first
> > field anyway.
> >
> > Let's drop it, precalculate header len and store within vi instead.
> >
> > This way we can also remove struct skb_vnet_hdr.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/net/virtio_net.c | 90 ++++++++++++++++++++++--------------------------
> > 1 file changed, 41 insertions(+), 49 deletions(-)
> >
>
> > @@ -260,13 +256,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >
> > hdr = skb_vnet_hdr(skb);
> >
> > - if (vi->mergeable_rx_bufs) {
> > - hdr_len = sizeof hdr->mhdr;
> > - hdr_padded_len = sizeof hdr->mhdr;
> > - } else {
> > - hdr_len = sizeof hdr->hdr;
> > + hdr_len = vi->hdr_len;
> > + if (vi->mergeable_rx_bufs)
> > + hdr_padded_len = sizeof *hdr;
>
> sizeof without () looks weird to me.
It's how it was originally, so I think I'd rather keep it.
I kind of like it too: it stresses the fact that sizeof is not a function
and so does not actually dereference the pointer
even though it looks like it does.
We can change it with a patch on top, but should then be changed
everywhere in this file.
> > + else
> > hdr_padded_len = sizeof(struct padded_vnet_hdr);
> > - }
> >
> > memcpy(hdr, p, hdr_len);
> >
>
> Otherwise:
>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* Re: [PATCH iproute2] iplink: allow to show ip addresses
From: Jiri Benc @ 2014-11-26 13:12 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Nicolas Dichtel, shemminger, netdev
In-Reply-To: <20141125061023.GA8819@unicorn.suse.cz>
On Tue, 25 Nov 2014 07:10:23 +0100, Michal Kubecek wrote:
> On Mon, Nov 24, 2014 at 05:42:17PM +0100, Nicolas Dichtel wrote:
> > This patch adds a new option (-addresses) to the 'ip link' command so that the
> > user can display link details and IP addresses with the same command.
> >
> > Example:
> > $ ip -d -a l ls gre1
> > 9: gre1@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1468 qdisc noqueue state UNKNOWN mode DEFAULT group default
> > link/gre 10.16.0.249 peer 10.16.0.121 promiscuity 0
> > gre remote 10.16.0.121 local 10.16.0.249 ttl inherit ikey 0.0.0.10 okey 0.0.0.10 icsum ocsum
> > inet 192.168.0.249 peer 192.168.0.121/32 scope global gre1
> > valid_lft forever preferred_lft forever
> > inet6 fe80::5efe:a10:f9/64 scope link
> > valid_lft forever preferred_lft forever
>
> Perhaps it would be more consistent to add -d option to "ip addr show"
> instead as we already have -s for statistics there (commit 5d5cf1b43).
Agreed.
Jiri
--
Jiri Benc
^ permalink raw reply
* Re: [PATCH v4 24/42] virtio_net: enable v1.0 support
From: Michael S. Tsirkin @ 2014-11-26 13:28 UTC (permalink / raw)
To: Cornelia Huck
Cc: rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126140857.6ff85e4b.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 02:08:57PM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 18:43:06 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > Now that we have completed 1.0 support, enable it in our driver.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/net/virtio_net.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
>
> Hm. The spec states that mac is driver-writable in the legacy case.
> Don't we need to fence writing it in virtnet_set_mac_address() in the
> virtio 1.0 case?
You are right. I'll add a patch to fix that one: we
should return -EOPNOTSUPP unless
VIRTIO_NET_F_CTRL_MAC_ADDR or
VERSION_1 is clear and VIRTIO_NET_F_MAC is set.
^ 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