* Re: virtio_net: ethtool supported link modes
From: Radu Rendec @ 2017-09-04 14:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev
In-Reply-To: <20170901204222-mutt-send-email-mst@kernel.org>
On Fri, 2017-09-01 at 20:45 +0300, Michael S. Tsirkin wrote:
> On Fri, Sep 01, 2017 at 05:19:53PM +0100, Radu Rendec wrote:
> > On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> > > > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > > > and duplex can be set to any valid value. The driver will "remember"
> > > > them and report them back in virtnet_get_link_ksettings.
> > > >
> > > > However, the supported link modes (link_modes.supported in struct
> > > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > > > setting is supported.
> > > >
> > > > Does it make more sense to set (at least a few of) the supported link
> > > > modes, such as 10baseT_Half ... 10000baseT_Full?
> > > >
> > > > I would expect to see consistency between what is reported in
> > > > link_modes.supported and what can actually be set. Could you please
> > > > share your opinion on this?
> >
> > The use case behind my original question is very simple:
> > * Net device is queried via ethtool for supported modes.
> > * Supported modes are presented to user.
> > * User can configure any of the supported modes.
>
> Since this has no effect on virtio, isn't presenting
> "no supported modes" to user the right thing to do?
Yes, that makes sense.
> > This is done transparently to the net device type (driver), so it
> > actually makes sense for physical NICs.
> >
> > This alone of course is not a good enough motivation to modify the
> > driver. And it can be easily addressed in user-space at the application
> > level by testing for the driver.
>
> I think you might want to special-case no supported modes.
> Special-casing virtio is probably best avoided.
>
> > I was merely trying to avoid driver-specific workarounds (i.e. keep the
> > application driver agnostic)
>
> I think that's the right approach. So if driver does not present
> any supported modes this probably means it is not necessary
> to display or program any.
Yes, apparently it boils down to special-casing no supported modes.
This avoids both modifying virtio and special-casing virtio, and keeps
the application driver-agnostic at the same time.
Thanks for all the feedback. It was very helpful in figuring out the
right approach. I really appreciate it.
Radu
^ permalink raw reply
* [PATCH] net: ipv6: fix regression of no RTM_DELADDR sent after DAD failure
From: Mike Manning @ 2017-09-04 14:52 UTC (permalink / raw)
To: netdev; +Cc: Mahesh Bandewar
Commit f784ad3d79e5 ("ipv6: do not send RTM_DELADDR for tentative
addresses") incorrectly assumes that no RTM_NEWADDR are sent for
addresses in tentative state, as this does happen for the standard
IPv6 use-case of DAD failure, see the call to ipv6_ifa_notify() in
addconf_dad_stop(). So as a result of this change, no RTM_DELADDR is
sent after DAD failure for a link-local when strict DAD (accept_dad=2)
is configured, or on the next admin down in other cases. The absence
of this notification breaks backwards compatibility and causes problems
after DAD failure if this notification was being relied on. The
solution is to allow RTM_DELADDR to still be sent after DAD failure.
Fixes: f784ad3d79e5("ipv6: do not send RTM_DELADDR for tentative addresses")
Signed-off-by: Mike Manning <mmanning@brocade.com>
Cc: Mahesh Bandewar <maheshb@google.com>
---
net/ipv6/addrconf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 936e9ab..ba757c2 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4982,9 +4982,10 @@ static void inet6_ifa_notify(int event, struct inet6_ifaddr *ifa)
/* Don't send DELADDR notification for TENTATIVE address,
* since NEWADDR notification is sent only after removing
- * TENTATIVE flag.
+ * TENTATIVE flag, if DAD has not failed.
*/
- if (ifa->flags & IFA_F_TENTATIVE && event == RTM_DELADDR)
+ if (ifa->flags & IFA_F_TENTATIVE && !(ifa->flags & IFA_F_DADFAILED) &&
+ event == RTM_DELADDR)
return;
skb = nlmsg_new(inet6_ifaddr_msgsize(), GFP_ATOMIC);
--
2.1.4
^ permalink raw reply related
* RE: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: David Laight @ 2017-09-04 14:49 UTC (permalink / raw)
To: 'Phil Sutter', Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20170901165256.21459-2-phil@nwl.cc>
From: Phil Sutter
> Sent: 01 September 2017 17:53
> By making use of strncpy(), both implementations are really simple so
> there is no need to add libbsd as additional dependency.
>
...
> +
> +size_t strlcpy(char *dst, const char *src, size_t size)
> +{
> + if (size) {
> + strncpy(dst, src, size - 1);
> + dst[size - 1] = '\0';
> + }
> + return strlen(src);
> +}
Except that isn't really strlcpy().
Better would be:
len = strlen(src) + 1;
if (len <= size)
memcpy(dst, src, len);
else if (size) {
dst[size - 1] = 0;
memcpy(dst, src, size - 1);
}
return len - 1;
WTF strlcpy() has that return value I don't know.
David
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 14:45 UTC (permalink / raw)
To: Jiri Benc
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20170904161303.69624386@griffin>
> On Mon, 4 Sep 2017 14:07:45 +0000, Jan Scheurich wrote:
> > Then perhaps I misunderstood your comment. I thought you didn't like that the
> > SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.
> > I was aiming to avoid this by lifting the two components of the NSH header
> > components to the top level:
> > OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER
> > OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT
>
> No, this should be a nested attr.
>
> I objected to the way value+mask combo is handled.
OK, sorry for the confusion.
So what is the correct layout for MASKED_SET action with nested fields?
1. All nested values, followed by all nested masks, or
2. For each nested field value followed by mask?
I guess alternative 1, but just to be sure.
Jan
^ permalink raw reply
* [PATCH net-next] rxrpc: Make service connection lookup always check for retry
From: David Howells @ 2017-09-04 14:28 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
When an RxRPC service packet comes in, the target connection is looked up
by an rb-tree search under RCU and a read-locked seqlock; the seqlock retry
check is, however, currently skipped if we got a match, but probably
shouldn't be in case the connection we found gets replaced whilst we're
doing a search.
Make the lookup procedure always go through need_seqretry(), even if the
lookup was successful. This makes sure we always pick up on a write-lock
event.
On the other hand, since we don't take a ref on the object, but rely on RCU
to prevent its destruction after dropping the seqlock, I'm not sure this is
necessary.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/conn_service.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/rxrpc/conn_service.c b/net/rxrpc/conn_service.c
index e60fcd2a4a02..f6fcdb3130a1 100644
--- a/net/rxrpc/conn_service.c
+++ b/net/rxrpc/conn_service.c
@@ -50,12 +50,11 @@ struct rxrpc_connection *rxrpc_find_service_conn_rcu(struct rxrpc_peer *peer,
else if (conn->proto.index_key > k.index_key)
p = rcu_dereference_raw(p->rb_right);
else
- goto done;
+ break;
conn = NULL;
}
} while (need_seqretry(&peer->service_conn_lock, seq));
-done:
done_seqretry(&peer->service_conn_lock, seq);
_leave(" = %d", conn ? conn->debug_id : -1);
return conn;
^ permalink raw reply related
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 14:13 UTC (permalink / raw)
To: Jan Scheurich
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D787F4EEB-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
On Mon, 4 Sep 2017 14:07:45 +0000, Jan Scheurich wrote:
> Then perhaps I misunderstood your comment. I thought you didn't like that the
> SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.
> I was aiming to avoid this by lifting the two components of the NSH header
> components to the top level:
> OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER
> OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT
No, this should be a nested attr.
I objected to the way value+mask combo is handled.
> See above. The two would be separate values in the same enum, i.e. distinct.
This is not how netlink attrs should be designed.
> Not sure I have the full picture here. Are you saying that the tc tool uses the same
> Netlink API to the kernel as OVS? What would be the back-end for tc? Also the
> openvswitch kernel module?
It does not use the same API but it makes sense for these two to share
common code. Plus hw offloading in ovs is done using tc.
Jiri
^ permalink raw reply
* RE: [PATCH net-next v7] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 14:07 UTC (permalink / raw)
To: Jiri Benc
Cc: Yang, Yi, netdev@vger.kernel.org, davem@davemloft.net,
dev@openvswitch.org, e@erig.me, blp@ovn.org
In-Reply-To: <20170904151601.2f198a53@griffin>
> > So is what you are suggesting the following?
> >
> > For matching:
> > OVS_KEY_ATTR_NSH_BASE_HEADER mandatory
> > OVS_KEY_ATTR_NSH_MD1_CONTEXT optional, in case MDTYPE == MD1
>
> This needs to be:
>
> OVS_KEY_ATTR_NSH
> OVS_KEY_ATTR_NSH_BASE_HEADER
> OVS_KEY_ATTR_NSH_MD1_CONTEXT
>
> > For setting:
> > OVS_ACTION_ATTR_SET_MASKED
> > - OVS_KEY_ATTR_NSH_BASE_HEADER when setting any base header fields
> > OVS_ACTION_ATTR_SET_MASKED
> > - OVS_KEY_ATTR_NSH_MD1_CONTEXT when setting any MD1 context data fields
>
> This needs to be:
>
> OVS_ACTION_ATTR_SET_MASKED
> OVS_KEY_ATTR_NSH
> OVS_KEY_ATTR_NSH_BASE_HEADER
> OVS_KEY_ATTR_NSH_MD1_CONTEXT
>
Then perhaps I misunderstood your comment. I thought you didn't like that the
SET_MASKED action wrapped OVS_KEY_ATTR_NSH which in itself was nested.
I was aiming to avoid this by lifting the two components of the NSH header
components to the top level:
OVS_NSH_ATTR_BASE_HEADER --> OVS_KEY_ATTR_NSH_BASE_HEADER
OVS_NSH_ATTR_MD1_CONTEXT --> OVS_KEY_ATTR_NSH_MD1_CONTEXT
> In your example, there's no way to distinguish OVS_KEY_ATTR_ENCAP and
> OVS_KEY_ATTR_NSH_BASE_HEADER, they are both "1".
See above. The two would be separate values in the same enum, i.e. distinct.
> > Should we consider to stick to that simple and efficient approach? As far
> > As I can see it will be generic for the foreseeable future.
>
> I'm not sure. From the kernel point of view, we want the same
> functionality offered by the openvswitch module and by a tc action.
>
> In theory, it can be the tc tool that constructs the header from the
> command line but there's no precedent. Dunno.
Not sure I have the full picture here. Are you saying that the tc tool uses the same
Netlink API to the kernel as OVS? What would be the back-end for tc? Also the
openvswitch kernel module?
BR, Jan
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Hannes Frederic Sowa @ 2017-09-04 13:50 UTC (permalink / raw)
To: Tom Herbert
Cc: Saeed Mahameed, Saeed Mahameed, David S. Miller,
Linux Netdev List
In-Reply-To: <CALx6S356vRYfcsN6FaRmpn2oC0GN9-JwLsF7ybC+w4sEGGsa7w@mail.gmail.com>
Tom Herbert <tom@herbertland.com> writes:
> An encapsulator sets the UDP source port to be the flow entropy of the
> packet being encapsulated. So when the packet traverses the network
> devices can base their hash just on the canonical 5-tuple which is
> sufficient for ECMP and RSS. IPv6 flow label is even better since the
> middleboxes don't even need to look at the transport header, a packet
> is steered based on the 3-tuple of addresses and flow label. In the
> Linux stack, udp_flow_src_port is used by UDP encapsulations to set
> the source port. Flow label is similarly set by ip6_make_flowlabel.
> Both of these functions use the skb->hash which is computed by calling
> flow dissector at most once per packet (if the packet was received
> with an L4 HW hash or locally originated on a connection the hash does
> not need to be computed).
This would require the MPLS stack copying the flowlabel of IPv6
connections between MPLS routers over their whole lifetime in the MPLS
network. The same would hold for MPLS encapsulated inside UDP, the
source port needs to be kept constant. This is very impractical. The
hash for the flow label can often not be recomputed by interim routers,
because they might lack the knowledge of the upper layer protocol.
UDP source port entropy still has the problem that we don't respect the
source port as RSS entropy by default in network cards, because of
possible fragmentation and thus possible reordering of packets. GRE does
not have this problem and is way easier to identify by hardware.
Basically we need to tell network cards that they can use specific
destination UDP ports where we allow the source port to be used in RSS
hash calculation. I don't see how this is any easier than just using GRE
with a defined protocol field? I do like the combination of ipv6
flowlabel + GRE.
Btw. people are using the GRE Key as additional entropy without looking
into the GRE payload.
Bye,
Hannes
^ permalink raw reply
* Re: EINVAL when using connect() for udp sockets
From: Daurnimator @ 2017-09-04 13:23 UTC (permalink / raw)
To: Eric Dumazet
Cc: Cong Wang, Linux Kernel Network Developers, William Ahern,
Santi T., Michael Kerrisk-manpages
In-Reply-To: <1493354932.31837.9.camel@edumazet-glaptop3.roam.corp.google.com>
On 28 April 2017 at 14:48, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2017-04-28 at 12:55 +1000, Daurnimator wrote:
>> On 1 April 2017 at 03:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > Please submit your patch formally and with a man page patch too.
>>
>> Did a patch get submitted? I had a look but couldn't see it.
>
> Not yet.
>
> I will probably wait for next cycle (linux-4.13), I have been busy on
> other work lately.
Will you have time to work on this soon?
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 13:16 UTC (permalink / raw)
To: Jan Scheurich
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D787F4E84-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
On Mon, 4 Sep 2017 12:57:15 +0000, Jan Scheurich wrote:
> So is what you are suggesting the following?
>
> For matching:
> OVS_KEY_ATTR_NSH_BASE_HEADER mandatory
> OVS_KEY_ATTR_NSH_MD1_CONTEXT optional, in case MDTYPE == MD1
This needs to be:
OVS_KEY_ATTR_NSH
OVS_KEY_ATTR_NSH_BASE_HEADER
OVS_KEY_ATTR_NSH_MD1_CONTEXT
> For setting:
> OVS_ACTION_ATTR_SET_MASKED
> - OVS_KEY_ATTR_NSH_BASE_HEADER when setting any base header fields
> OVS_ACTION_ATTR_SET_MASKED
> - OVS_KEY_ATTR_NSH_MD1_CONTEXT when setting any MD1 context data fields
This needs to be:
OVS_ACTION_ATTR_SET_MASKED
OVS_KEY_ATTR_NSH
OVS_KEY_ATTR_NSH_BASE_HEADER
OVS_KEY_ATTR_NSH_MD1_CONTEXT
In your example, there's no way to distinguish OVS_KEY_ATTR_ENCAP and
OVS_KEY_ATTR_NSH_BASE_HEADER, they are both "1".
> Should we consider to stick to that simple and efficient approach? As far
> As I can see it will be generic for the foreseeable future.
I'm not sure. From the kernel point of view, we want the same
functionality offered by the openvswitch module and by a tc action.
In theory, it can be the tc tool that constructs the header from the
command line but there's no precedent. Dunno.
Jiri
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 13:10 UTC (permalink / raw)
To: Yi Yang
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA, e,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <1504096752-102003-1-git-send-email-yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On Wed, 30 Aug 2017 20:39:12 +0800, Yi Yang wrote:
> +enum ovs_nsh_key_attr {
> + OVS_NSH_KEY_ATTR_BASE, /* struct ovs_nsh_key_base. */
> + OVS_NSH_KEY_ATTR_MD1, /* struct ovs_nsh_key_md1. */
> + OVS_NSH_KEY_ATTR_MD2, /* variable-length octets for MD type 2. */
> + __OVS_NSH_KEY_ATTR_MAX
> +};
One more thing: 0 is reserved, the first attr must be
OVS_NSH_KEY_ATTR_UNSPEC.
Jiri
^ permalink raw reply
* RE: [PATCH net-next v7] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 12:57 UTC (permalink / raw)
To: Jiri Benc, Yang, Yi
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org
In-Reply-To: <20170904124216.6db68e8c@griffin>
> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:
> > I think we have had similiar discussion about this, the issue is we have
> > no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
> >
> > So we have to do so, there is miniflow to handle such issue in
> > userspace, but kernel data path didn't provide such mechanism. I think
> > every newly-added key will result in the same space-wasting issue for
> > kernel data path, isn't it?
>
> Yes. And it would be surprising if it didn't have an effect on
> performance.
>
> I don't care that much as this is a result of openvswitch module design
> decisions but it still would be good to reach a consensus before
> implementing uAPI that might not be needed in the end.
Matching on NSH MD1 context headers is definitely required. So these must
be part of the flow.
>
> > OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
> > action, so no reference code for this, set action has two use cases, one
> > has mask but the other hasn't, so it will be easier an nested attribute is
> > handled as a whole, in user space, nsh_key_from_nlattr is used in
> > several places.
>
> I very much disagree. What you're doing is very poor design as can be
> seen from the code where you have to do weird gymnastics to implement
> that. Compare that to a much simple case where each attribute carries
> its own value and mask.
I have no principle objections against splitting NSH base header and MD1 context
headers into independent key attributes on the uAPI if that simplifies the code.
But we need to full picture here:
So is what you are suggesting the following?
For matching:
OVS_KEY_ATTR_NSH_BASE_HEADER mandatory
OVS_KEY_ATTR_NSH_MD1_CONTEXT optional, in case MDTYPE == MD1
For setting:
OVS_ACTION_ATTR_SET_MASKED
- OVS_KEY_ATTR_NSH_BASE_HEADER when setting any base header fields
OVS_ACTION_ATTR_SET_MASKED
- OVS_KEY_ATTR_NSH_MD1_CONTEXT when setting any MD1 context data fields
For push_nsh:
OVS_ACTION_ATTR_PUSH_NSH
- OVS_KEY_ATTR_NSH_BASE_HEADER mandatory
- OVS_NSH_ATTR_CONTEXT_HEADERS optional (opaque metadata octet stream)
So push_nsh would still require nested attributes. Is that what we want (see below).
> > How can we do so? I see batch process code in user space implementation,
> > but kernel data path didn't such infrastructure,
>
> You're right that there's no infrastructure. I somewhat agree that it
> might be out of scope of this patch and it can be optimized later.
> That's why I also included other suggestions to speed this up until we
> implement better parsing: namely, verify correctness once (at the time
> it is set from user space) and just expect things to be correct in the
> fast path.
>
> > how can we know next push_nsh uses the same nsh header as previous
> > one?
>
> We store the prepopulated header together with the action.
I agree.
In our original modelling of OVS_ACTION_ATTR_PUSH_NSH in OVS 2.8 we
introduced a monolithic push_nsh action struct including the NSH base header
and variable length opaque context headers. That avoids any logic in the
kernel datapath to translate a nested action attribute into an internal format
with pre-populated header. The user space does that work.
Should we consider to stick to that simple and efficient approach? As far
As I can see it will be generic for the foreseeable future.
> > > These checks should be done only once when the action is configured, not for
> > > each packet.
> >
> > I don't know how to implement a batch processing in kernel data path, it
> > seems there isn't such code for reference.
>
> The checks should be done somewhere in __ovs_nla_copy_action. Which
> seems to be done even in your patch by nsh_key_put_from_nlattr
> (I didn't get that far during the review last week. The names of
> those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to
> tell what they do without looking at the call sites - something you
> should also consider to improve). That makes it even more wrong: you
> appear to check everything twice, first on configuration and then for
> every packet.
Agree. Validation should only happen at action configuration time.
BR, Jan
^ permalink raw reply
* stable-kernel-rules: wireless and netdev-FAQ
From: Kalle Valo @ 2017-09-04 12:58 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Larry Finger, Helmut Schaa, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170902082223.GA11293@redhat.com>
(adding netdev and lkml)
Stanislaw Gruszka <sgruszka@redhat.com> writes:
> On Fri, Sep 01, 2017 at 05:31:57PM +0300, Kalle Valo wrote:
>> Stanislaw Gruszka <sgruszka@redhat.com> writes:
>>
>> > On Thu, Aug 31, 2017 at 10:33:28AM -0500, Larry Finger wrote:
>> >> Should the patch to wireless-drivers be annotated with a Stable reference so
>> >> that it is added to 4.12 and 4.13?
>> >
>> > According to Documentation/networking/netdev-FAQ.txt networking patches
>> > should not be marked cc:stable, instead a decent commit log should
>> > be written describing a bugfix. Which I believe it is done for
>> > this patch.
>>
>> But that's for net and net-next trees, not for wireless trees. With
>> wireless patches we use "Cc: stable@..." references.
>
> Oh, ok. I was confused by below part of
> Documentation/process/stable-kernel-rules.rst
> (because wireless drivers are located in drivers/net/)
>
> - If the patch covers files in net/ or drivers/net please follow netdev stable
> submission guidelines as described in
> Documentation/networking/netdev-FAQ.txt
Yeah, that's confusing and should be clarified that wireless follows
traditional stable process. IIRC bluetooth does the same so it's not
just wireless.
Any volunteers to fix it? :)
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 12:57 UTC (permalink / raw)
To: Yang, Yi
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170904120907.GA75461@cran64.bj.intel.com>
On Mon, 4 Sep 2017 20:09:07 +0800, Yang, Yi wrote:
> So we must do many changes if we want to break this assumption.
We may do as many changes as we want to. This is uAPI we're talking
about and we need to get it right since the beginning. Sure, it may
mean that some user space programs need some changes in order to make
use of the new features. That happens every day.
I also don't understand where's the problem. It's very easy to check
for NLA_F_NESTED and generically act based on that in the function you
quoted. Just call a different function than format_odp_key_attr to
handle ovs_nsh_key_attr attributes in case the nested flag is set and
the attribute is OVS_KEY_ATTR_NSH and you're done. You'll need such
function anyway, it's not much different code size wise to call it from
format_odp_key_attr or from format_odp_action.
Jiri
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Yang, Yi @ 2017-09-04 12:09 UTC (permalink / raw)
To: Jiri Benc
Cc: netdev@vger.kernel.org, davem@davemloft.net, dev@openvswitch.org,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170904124216.6db68e8c@griffin>
On Mon, Sep 04, 2017 at 12:42:16PM +0200, Jiri Benc wrote:
> On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:
> > I think we have had similiar discussion about this, the issue is we have
> > no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
> >
> > So we have to do so, there is miniflow to handle such issue in
> > userspace, but kernel data path didn't provide such mechanism. I think
> > every newly-added key will result in the same space-wasting issue for
> > kernel data path, isn't it?
>
> Yes. And it would be surprising if it didn't have an effect on
> performance.
>
> I don't care that much as this is a result of openvswitch module design
> decisions but it still would be good to reach a consensus before
> implementing uAPI that might not be needed in the end.
>
> > OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
> > action, so no reference code for this, set action has two use cases, one
> > has mask but the other hasn't, so it will be easier an nested attribute is
> > handled as a whole, in user space, nsh_key_from_nlattr is used in
> > several places.
>
> I very much disagree. What you're doing is very poor design as can be
> seen from the code where you have to do weird gymnastics to implement
> that. Compare that to a much simple case where each attribute carries
> its own value and mask.
>
> > If we change it per your proposal, there will be a lot
> > of rework,
>
> That's not an argument. We care about doing things right, we don't do
> things hastily and with as little effort as possible.
Jiri, I check OVS source code carefully, if you check OVS code in
format_odp_action in lib/odp-util.c, it has a strong assumption for set
action, mask is following value no matter it is simple attribute or
nested attribute, I copy source code here for your reference,
case OVS_ACTION_ATTR_SET_MASKED:
a = nl_attr_get(a);
size = nl_attr_get_size(a) / 2;
ds_put_cstr(ds, "set(");
/* Masked set action not supported for tunnel key, which is
* bigger. */
if (size <= sizeof(struct ovs_key_ipv6)) {
struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct
ovs_key_ipv6),
sizeof(struct nlattr))];
struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct
ovs_key_ipv6),
sizeof(struct nlattr))];
mask->nla_type = attr->nla_type = nl_attr_type(a);
mask->nla_len = attr->nla_len = NLA_HDRLEN + size;
memcpy(attr + 1, (char *)(a + 1), size);
memcpy(mask + 1, (char *)(a + 1) + size, size);
format_odp_key_attr(attr, mask, NULL, ds, false);
} else {
format_odp_key_attr(a, NULL, NULL, ds, false);
}
ds_put_cstr(ds, ")");
break;
So we must do many changes if we want to break this assumption.
>
> > we have to provide two functions to handle this, one is for
> > the case with mask, the other is for the case without mask.
>
> I don't see that. The function is called from a single place only.
>
> > How can we do so? I see batch process code in user space implementation,
> > but kernel data path didn't such infrastructure,
>
> You're right that there's no infrastructure. I somewhat agree that it
> might be out of scope of this patch and it can be optimized later.
> That's why I also included other suggestions to speed this up until we
> implement better parsing: namely, verify correctness once (at the time
> it is set from user space) and just expect things to be correct in the
> fast path.
>
> > how can we know next push_nsh uses the same nsh header as previous
> > one?
>
> We store the prepopulated header together with the action.
>
> > > Shouldn't we reject the packet, then? Pretending that something was parsed
> > > correctly while in fact it was not, does not look correct.
> >
> > For MD type 2, we don't implement metadata parsing, but it can still
> > work. I'm not sure what you mean here, how do we reject a packet here?
>
> Okay, we can match on mdtype and thus can find out about this type of
> packets. No problem, then.
>
> > > These checks should be done only once when the action is configured, not for
> > > each packet.
> >
> > I don't know how to implement a batch processing in kernel data path, it
> > seems there isn't such code for reference.
>
> The checks should be done somewhere in __ovs_nla_copy_action. Which
> seems to be done even in your patch by nsh_key_put_from_nlattr
> (I didn't get that far during the review last week. The names of
> those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to
> tell what they do without looking at the call sites - something you
> should also consider to improve). That makes it even more wrong: you
> appear to check everything twice, first on configuration and then for
> every packet.
Ok, I'll carefully remove unnecessary checks in next version.
>
> Jiri
^ permalink raw reply
* RE: [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-09-04 11:57 UTC (permalink / raw)
To: Hannes Frederic Sowa, Yang, Yi
Cc: netdev@vger.kernel.org, dev@openvswitch.org, jbenc@redhat.com,
e@erig.me, blp@ovn.org
In-Reply-To: <87mv6abte5.fsf@stressinduktion.org>
> >> Does it makes sense to keep the context headers as part of the flow?
> >> What is the reasoning behind it? With mdtype 2 headers this might either
> >> not work very well or will increase sw_flow_key size causing slowdowns
> >> for all protocols.
> >
> > For userspace, miniflow can handle such issue, but kernel data path
> > didn't provide such mechanism, so I don't think we can think of a better
> > way to fix this.
>
> I do think that both, kernel and dpdk dp, are fine if you don't match on
> context fields, which I think is the common case.
Agree
> As soon as a few paths start to match on contexts at different offsets I
> am not sure if miniflow will actually help. I don't know enough about
> that. Set cover is a NP hard problem, so constructing plain simple flows
> will be an approximation somewhere anyway at some point.
>
> If the context values are in some way discrete it might make sense, but
> for load balancing purposes your ingress router might fill out one
> context field with a flow hash of the inner flow to allow ECMP or
> loadbalancing. You might want to treat that separately. Otherwise the
> different paths might always need to context[3] & 0x3 (bits as number of
> possible next hops) and put that into the flow state. Because every hop
> and every path might have different numbers of nexthops etc., I don't
> think this is easy unifiable anymore.
Yes, that would become a scaling challenge for the datapaths and also for
schemes to off-load that datapath to HW. I believe it requires discipline
on the controller side to not let the number of needed masks explode for
OVS.
> > I believe every newly-added key will result in similiar issue you
> > concern, maybe it will be better to introduce miniflow in kernel data
> > path, but it isn't in scope of this patch series.
>
> You will see the same problem with offloading flows e.g. to network
> cards very soon. Flow explosion here will cause your 100Gbit/s NIC
> suddenly to go to software slow path.
>
> > It will be great if you can have a better proposal to fix your concern.
>
> I do think that something alike miniflow might make sense, but I don't
> know enough about miniflow.
Struct miniflow in the netdev datapath is merely a compact representation
of struct flow. struct flow is partitioned in to 64-bit "stripes" and struct
miniflow only stores those 64-bit bit stripes that have a non-zero mask stripe.
It reduces the memory footprint for storing packet flow and megaflow entries
but does not provide any megaflow lookup performance as such.
> New ACTIONS, that are not only bound to NSH, seem appealing to me at the
> moment. E.g. have a subtable match in the kernel dp or allowing for some
> OXM % modulo -> select action or neighbor alike thing would probably
> cover a lot of cases in the beginning. The submatch would probably very
> much reassemble the miniflow in dpdk dp.
I would like to discuss your ideas to optimize scalable load-sharing in OVS.
But I think we should do that in a separate mail thread to let the immediate
NSH kernel datapath work proceed.
Can you provide some more details of what you suggest? Perhaps take that
on the ovs-dev mailing list.
BR, Jan
^ permalink raw reply
* Re: [ethtool] ethtool: Remove UDP Fragmentation Offload use from ethtool
From: Tariq Toukan @ 2017-09-04 11:46 UTC (permalink / raw)
To: Michal Kubecek, Tariq Toukan
Cc: John W. Linville, Eric Dumazet, David Miller, netdev,
Eran Ben Elisha, Shaker Daibes
In-Reply-To: <20170829104452.b6n5ikxxioib7766@unicorn.suse.cz>
On 29/08/2017 1:44 PM, Michal Kubecek wrote:
> On Tue, Aug 29, 2017 at 10:50:20AM +0300, Tariq Toukan wrote:
>> On 28/08/2017 9:22 PM, John W. Linville wrote:
>>> On Mon, Aug 28, 2017 at 08:00:11AM -0700, Eric Dumazet wrote:
>>>> On Mon, 2017-08-28 at 15:38 +0300, Tariq Toukan wrote:
>>>>> From: Shaker Daibes <shakerd@mellanox.com>
>>>>>
>>>>> UFO was removed in kernel, here we remove it in ethtool app.
>>>>>
>>>>> Fixes the following issue:
>>>>> Features for ens8:
>>>>> Cannot get device udp-fragmentation-offload settings: Operation not supported
>>
>> But I wonder how the warning removal should be done??
>>
>> I have some suggestions in mind:
>> 1) Have a special condition that does not print a warning only in the case
>> of UFO?
>> 2) Remove the warning totally? I don't like this option.
>> 3) Add a max_kernel_ver field in struct off_flag_def, and use it to not
>> print the warning, or to mark the feature 'off [fixed]'.
>
> IMHO there is nothing wrong with not writing a warning for "get"
> operation, after all it's just "ethtool -k", i.e. "show me all
> offloading flags" and we do not warn about unsupported named features
> either. IMHO the only question should be how friendly we should be to
> old scripts expecting the line in the output:
>
> (a) omit the "udp-fragmentation-offload:" line (be consistent)
> (b) say something like "udp-fragmentation-offload: n/a"
> (c) pretend it's there and is off (most careful but misleading)
>
> Personally, I would prefer (a) but some badly written scripts might have
> problem with this approach.
>
Thanks Michal.
I think we'll go for some variation of (a), omitting the warning.
Here we can choose to limit the omission for this exact case, i.e. only
when offload is UFO and err is "Operation not supported".
> On the other hand, an attempt to _set_ the flag with "ethtool -K" should
> issue an error.
>
> Michal Kubecek
>
^ permalink raw reply
* Re: [ovs-dev] [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Hannes Frederic Sowa @ 2017-09-04 11:45 UTC (permalink / raw)
To: Jan Scheurich
Cc: Mooney, Sean K, dev@openvswitch.org, e@erig.me, jbenc@redhat.com,
netdev@vger.kernel.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D787F4CB0@ESESSMB107.ericsson.se>
Hello,
Jan Scheurich <jan.scheurich@ericsson.com> writes:
>> >> >> Does it makes sense to keep the context headers as part of the flow?
>> >> >> What is the reasoning behind it? With mdtype 2 headers this might
>> >> >> either not work very well or will increase sw_flow_key size causing
>> >> >> slowdowns for all protocols.
>> >> > [Mooney, Sean K]
>> >> > Having the nsh context headers in the flow is quite useful It would
>> >> > allow loadblancing on values stored in the context headers Or other
>> >> > use. I belive odl previously used context header 4 to store a Flow id
>> >> > so this could potentialy be used with the multipath action to have
>> >> ovs
>> >> > Choose between several possible next hops in the chain.
>> >>
>> >> In OVS, masks are a list(!) for matching. How can this work for
>> >> different paths that might require different masks? If they can't be
>> >> unified you even get exact matches. Thus, for OVS the context should
>> >> not be part of the flow.
>>
>> > [Mooney, Sean K] I'm not sure what you mean about a list as I never
>> > made reference to one. md type 1 context headers are 4 mandatory 32
>> > bit field.
>>
>> The semantic of the context fields depend on the path id. Every path can
>> have their own interpretation of context fields.
>>
>> Thus the paths will cetainly have their own masks. I hope I understood
>> this part of the standard correctly.
>>
>> dp only supports installing (approximated) disjoint megaflows and this
>> affects performance a lot. Every new mask that gets added to the dp will
>> be installed into a list which will be walked sequentially for
>> packets. This will kill performance.
>>
>> I assume that user space slows down, too, depending on the algorithm
>> they use generate megaflows.
>
> The primary use case for OVS in SFC/NSH is SFF. In almost all SFF use
> cases OVS will not need to match on context headers. The ability of OVS
> to match on context headers should not in general slow down the datapath.
The sw_flow_key is huge nowadays. You will be expanding it to the eigth
cache line. But I agree that it should not generally slow down the data
path.
> When SFC controllers match on parts of the context headers (e.g. in the
> final SFF or for load-balancing purposes), we will get additional megaflow
> masks. This is a fact of life in OVS and nothing new in NSH. I don't think
> this should prevent us from supporting valid use cases in OVS.
Other protocols are using entropy from the protocol and load balance on
that implicitly. Why not NSH?
I would argue that before NSH the masks were pretty constant. NSH
introduces context dependent paths where the design emphasizes to have
different masks per tenant. I don't think this is the case for other
protocols processed by the kernel dp right now. So the megaflow
unification was rather effective so far.
I expect a major slow down with just 10-20 masks.
Btw. this also affects possibilities to synthesize those flows to
hardware at all.
> The slow-down of the megaflow lookup in the datapath with the number
> of masks has been addressed in the user-space datapath with sorting the
> mask lists according to hit frequency and maintaining one sorted lists per
> ingress port. There is further work in progress to improve on this with a
> cuckoo hash to pick the most likely mask first.
> It should be possible to apply similar optimization schemes also in the
> Kernel datapath.
Lots of optimizations could be done, I agree, but the fundamental
problem still exists, especially if you want to be traffic agnostic
(short slow lived flows, 64 byte size UDP/RTP traffic etc., you
basically walk through more layers of abstraction to find your flow
resolution).
>
>> > form an ovs context they should be treated the same as the 32 bit register fields.
>> > We do not need to necessarily support those in md type 2 as all metadata is optional.
>
> Support for matching on or updating MD2 TLV context headers is FFS in a
> future OVS release. We do not foresee to add MD2 TLVs explicitly to the
> flow struct.
In my opinion I don't see a difference between MD1 and MD2.
>> > You can also see that context header 1 and 2 are still used in the newer odl netvirt sfc classifier implementation
>> > https://github.com/opendaylight/netvirt/blob/ba22f7cf19d8a827d77a3391a7f654344ade43d8/docs/specs/new-sfc-
>> classifier.rst#pipeline-changes
>> > so for odl existing nsh support to work with md type one we must have the ability to set the nsh context headers
>> > and should have the ability to match on them also for load lancing between service function in service function group.
>>
>> As I said, a separate action sounds much more useful.
>
> I don't think it is a good approach in OVS to introduce custom actions
> for NSH, e.g. for load sharing based on context header data. The
> primary tool for load sharing in OpenFlow is the Select Group. OVS
> already support an extension to the OF 1.5 Group Mod message to
> specify which fields to hash on. This can be used to hash on the
> relevant NSH context headers.
It doesn't need to be custom to NSH at all. A load balancing action
based on a flow hash seems pretty common to me (I am talking about
kernel actions here).
Thanks and bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/2] netfilter/xt_hashlimit: new feature/algorithm for xt_hashlimit
From: Pablo Neira Ayuso @ 2017-09-04 11:31 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: kadlec, netfilter-devel, johunt, fw, netdev, pai.vishwain
In-Reply-To: <20170904101433.GA3979@salvia>
On Mon, Sep 04, 2017 at 12:14:33PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 18, 2017 at 04:58:59PM -0400, Vishwanath Pai wrote:
> [...]
> > The main difference between the existing algorithm and the new one is
> > that the existing algorithm rate-limits the flow whereas the new
> > algorithm does not. Instead it *classifies* the flow based on whether
> > it is above or below a certain rate. I will demonstrate this with an
> > example below. Let us assume this rule:
> >
> > iptables -A INPUT -m hashlimit --hashlimit-above 10/s -j new_chain
> >
> > If the packet rate is 15/s, the existing algorithm would ACCEPT 10
> > packets every second and send 5 packets to "new_chain".
> >
> > But with the new algorithm, as long as the rate of 15/s is sustained,
> > all packets will continue to match and every packet is sent to new_chain.
>
> Sounds good, applied, thanks.
BTW, for the record, I have rename this patch title to:
netfilter: xt_hashlimit: add rate match mode
^ permalink raw reply
* Re: [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Hannes Frederic Sowa @ 2017-09-04 11:22 UTC (permalink / raw)
To: Yang, Yi
Cc: netdev@vger.kernel.org, dev@openvswitch.org, jbenc@redhat.com,
e@erig.me, blp@ovn.org, jan.scheurich@ericsson.com
In-Reply-To: <20170904023831.GA68062@cran64.bj.intel.com>
Hello,
"Yang, Yi" <yi.y.yang@intel.com> writes:
> On Wed, Aug 30, 2017 at 05:53:27PM +0800, Hannes Frederic Sowa wrote:
>> Hello,
>>
>> Yi Yang <yi.y.yang@intel.com> writes:
>>
>> [...]
>>
>> > +struct ovs_key_nsh {
>> > + u8 flags;
>> > + u8 ttl;
>> > + u8 mdtype;
>> > + u8 np;
>> > + __be32 path_hdr;
>> > + __be32 context[NSH_MD1_CONTEXT_SIZE];
>> > +};
>> > +
>> > struct sw_flow_key {
>> > u8 tun_opts[IP_TUNNEL_OPTS_MAX];
>> > u8 tun_opts_len;
>> > @@ -144,6 +154,7 @@ struct sw_flow_key {
>> > };
>> > } ipv6;
>> > };
>> > + struct ovs_key_nsh nsh; /* network service header */
>> > struct {
>> > /* Connection tracking fields not packed above. */
>> > struct {
>>
>> Does it makes sense to keep the context headers as part of the flow?
>> What is the reasoning behind it? With mdtype 2 headers this might either
>> not work very well or will increase sw_flow_key size causing slowdowns
>> for all protocols.
>
> For userspace, miniflow can handle such issue, but kernel data path
> didn't provide such mechanism, so I don't think we can think of a better
> way to fix this.
I do think that both, kernel and dpdk dp, are fine if you don't match on
context fields, which I think is the common case.
As soon as a few paths start to match on contexts at different offsets I
am not sure if miniflow will actually help. I don't know enough about
that. Set cover is a NP hard problem, so constructing plain simple flows
will be an approximation somewhere anyway at some point.
If the context values are in some way discrete it might make sense, but
for load balancing purposes your ingress router might fill out one
context field with a flow hash of the inner flow to allow ECMP or
loadbalancing. You might want to treat that separately. Otherwise the
different paths might always need to context[3] & 0x3 (bits as number of
possible next hops) and put that into the flow state. Because every hop
and every path might have different numbers of nexthops etc., I don't
think this is easy unifiable anymore.
> We have to have context headers in flow for match and set, every hop in
> service function chaining can put its metedata into context headers on
> demand, MD type 2 is much more complicated than you can imagine, it is
> impossible to use an uniform way to handle MD type 1 and MD type 2, our
> way is to keep MD type 1 keys in struct ovs_key_nsh and to reuse struct
> flow_tnl for MD type 2 metadata, in case of MD type 2, we can set
> context headers to 0 in struct ovs_key_nsh.
Understood and agreed.
> I beleive every newly-added key will result in similiar issue you
> concern, maybe it will be better to introduce miniflow in kernel data
> path, but it isn't in scope of this patch series.
You will see the same problem with offloading flows e.g. to network
cards very soon. Flow explosion here will cause your 100Gbit/s NIC
suddenly to go to software slow path.
> It will be great if you can have a better proposal to fix your concern.
I do think that something alike miniflow might make sense, but I don't
know enough about miniflow.
New ACTIONS, that are not only bound to NSH, seem appealing to me at the
moment. E.g. have a subtable match in the kernel dp or allowing for some
OXM % modulo -> select action or neighbor alike thing would probably
cover a lot of cases in the beginning. The submatch would probably very
much reassemble the miniflow in dpdk dp.
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH net-next v7] openvswitch: enable NSH support
From: Jiri Benc @ 2017-09-04 10:42 UTC (permalink / raw)
To: Yang, Yi
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, e@erig.me,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20170904080005.GA70767-re2EX8HDrk21gSHoDXDV2kEOCMrvLtNR@public.gmane.org>
On Mon, 4 Sep 2017 16:00:05 +0800, Yang, Yi wrote:
> I think we have had similiar discussion about this, the issue is we have
> no way to handle both MD type 1 and MD type 2 by using a common flow key struct.
>
> So we have to do so, there is miniflow to handle such issue in
> userspace, but kernel data path didn't provide such mechanism. I think
> every newly-added key will result in the same space-wasting issue for
> kernel data path, isn't it?
Yes. And it would be surprising if it didn't have an effect on
performance.
I don't care that much as this is a result of openvswitch module design
decisions but it still would be good to reach a consensus before
implementing uAPI that might not be needed in the end.
> OVS_ATTR_KEY_NSH is the first case to use nested attributes for set
> action, so no reference code for this, set action has two use cases, one
> has mask but the other hasn't, so it will be easier an nested attribute is
> handled as a whole, in user space, nsh_key_from_nlattr is used in
> several places.
I very much disagree. What you're doing is very poor design as can be
seen from the code where you have to do weird gymnastics to implement
that. Compare that to a much simple case where each attribute carries
its own value and mask.
> If we change it per your proposal, there will be a lot
> of rework,
That's not an argument. We care about doing things right, we don't do
things hastily and with as little effort as possible.
> we have to provide two functions to handle this, one is for
> the case with mask, the other is for the case without mask.
I don't see that. The function is called from a single place only.
> How can we do so? I see batch process code in user space implementation,
> but kernel data path didn't such infrastructure,
You're right that there's no infrastructure. I somewhat agree that it
might be out of scope of this patch and it can be optimized later.
That's why I also included other suggestions to speed this up until we
implement better parsing: namely, verify correctness once (at the time
it is set from user space) and just expect things to be correct in the
fast path.
> how can we know next push_nsh uses the same nsh header as previous
> one?
We store the prepopulated header together with the action.
> > Shouldn't we reject the packet, then? Pretending that something was parsed
> > correctly while in fact it was not, does not look correct.
>
> For MD type 2, we don't implement metadata parsing, but it can still
> work. I'm not sure what you mean here, how do we reject a packet here?
Okay, we can match on mdtype and thus can find out about this type of
packets. No problem, then.
> > These checks should be done only once when the action is configured, not for
> > each packet.
>
> I don't know how to implement a batch processing in kernel data path, it
> seems there isn't such code for reference.
The checks should be done somewhere in __ovs_nla_copy_action. Which
seems to be done even in your patch by nsh_key_put_from_nlattr
(I didn't get that far during the review last week. The names of
those nsh_(key|hdr)_*_nlattr are so similar that it's impossible to
tell what they do without looking at the call sites - something you
should also consider to improve). That makes it even more wrong: you
appear to check everything twice, first on configuration and then for
every packet.
Jiri
^ permalink raw reply
* Re: [PATCH 1/2] netfilter/xt_hashlimit: new feature/algorithm for xt_hashlimit
From: Pablo Neira Ayuso @ 2017-09-04 10:14 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: kadlec, netfilter-devel, johunt, fw, netdev, pai.vishwain
In-Reply-To: <1503089939-15938-1-git-send-email-vpai@akamai.com>
On Fri, Aug 18, 2017 at 04:58:59PM -0400, Vishwanath Pai wrote:
[...]
> The main difference between the existing algorithm and the new one is
> that the existing algorithm rate-limits the flow whereas the new
> algorithm does not. Instead it *classifies* the flow based on whether
> it is above or below a certain rate. I will demonstrate this with an
> example below. Let us assume this rule:
>
> iptables -A INPUT -m hashlimit --hashlimit-above 10/s -j new_chain
>
> If the packet rate is 15/s, the existing algorithm would ACCEPT 10
> packets every second and send 5 packets to "new_chain".
>
> But with the new algorithm, as long as the rate of 15/s is sustained,
> all packets will continue to match and every packet is sent to new_chain.
Sounds good, applied, thanks.
A couple of questions: Does it really make sense to expose
--hashlimit-rate-interval or are you using 1 second always there? I
always wonder if it makes sense to expose yet another toggle that it's
not clear to me how the user would take advantage from this.
Just curious here.
^ permalink raw reply
* [PATCH v3 net-next 1/2] net: Export tcpv6_prot
From: Ilya Lesokhin @ 2017-09-04 10:14 UTC (permalink / raw)
To: netdev, davem; +Cc: davejwatson, aviadye, Ilya Lesokhin, Boris Pismenny
In-Reply-To: <1504520041-55634-1-git-send-email-ilyal@mellanox.com>
Want to be able to use these in TLS.
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
---
net/ipv6/tcp_ipv6.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 38f76d8..60d0629 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1948,6 +1948,7 @@ struct proto tcpv6_prot = {
#endif
.diag_destroy = tcp_abort,
};
+EXPORT_SYMBOL_GPL(tcpv6_prot);
/* thinking of making this const? Don't.
* early_demux can change based on sysctl.
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 net-next 0/2] Use correct sk->sk_prot for IPV6
From: Ilya Lesokhin @ 2017-09-04 10:13 UTC (permalink / raw)
To: netdev, davem; +Cc: davejwatson, aviadye, Ilya Lesokhin
The tls ulp overrides sk->prot with a new tls specific proto structs.
The tls specific structs were previously based on the ipv4 specific
tcp_prot sturct.
As a result, attaching the tls ulp to an ipv6 tcp socket replaced
some ipv6 callback with the ipv4 equivalents.
This patch adds ipv6 tls proto structs and uses them when
attached to ipv6 sockets.
Changed since v2:
- Dropped patch to fix IPV6_ADDRFORM setsockopt
There was some disagreement about the correct way of fixinig it,
and this series does not depend on it.
Changes since v1:
- TLS now dependes on IPV6
This fixes complication issues when TLS is built-in and IPV6 is a module.
The downside should be small as it is unlikely that there are kernel TLS
users who can't afford to include IPV6 in thier kernel.
- tls_init now checks sk->sk_prot directly
This is somewhat safer then checking indirectly through sk->sk_family
Ilya Lesokhin (2):
net: Export tcpv6_prot
tls: Use correct sk->sk_prot for IPV6
net/ipv6/tcp_ipv6.c | 1 +
net/tls/Kconfig | 1 +
net/tls/tls_main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
3 files changed, 40 insertions(+), 12 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH v3 net-next 2/2] tls: Use correct sk->sk_prot for IPV6
From: Ilya Lesokhin @ 2017-09-04 10:14 UTC (permalink / raw)
To: netdev, davem; +Cc: davejwatson, aviadye, Ilya Lesokhin, Boris Pismenny
In-Reply-To: <1504520041-55634-1-git-send-email-ilyal@mellanox.com>
The tls ulp overrides sk->prot with a new tls specific proto structs.
The tls specific structs were previously based on the ipv4 specific
tcp_prot sturct.
As a result, attaching the tls ulp to an ipv6 tcp socket replaced
some ipv6 callback with the ipv4 equivalents.
This patch adds ipv6 tls proto structs and uses them when
attached to ipv6 sockets.
Fixes: 3c4d7559159b ('tls: kernel TLS support')
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com>
---
net/tls/Kconfig | 1 +
net/tls/tls_main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/net/tls/Kconfig b/net/tls/Kconfig
index eb58303..7e9cf8b 100644
--- a/net/tls/Kconfig
+++ b/net/tls/Kconfig
@@ -7,6 +7,7 @@ config TLS
select CRYPTO
select CRYPTO_AES
select CRYPTO_GCM
+ select IPV6
default n
---help---
Enable kernel support for TLS protocol. This allows symmetric
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 60aff60..33c499e 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -40,13 +40,25 @@
#include <linux/sched/signal.h>
#include <net/tls.h>
+#include <net/transp_v6.h>
MODULE_AUTHOR("Mellanox Technologies");
MODULE_DESCRIPTION("Transport Layer Security Support");
MODULE_LICENSE("Dual BSD/GPL");
-static struct proto tls_base_prot;
-static struct proto tls_sw_prot;
+enum {
+ TLSV4,
+ TLSV6,
+ TLS_NUM_PROTS,
+};
+
+enum {
+ TLS_BASE_TX,
+ TLS_SW_TX,
+ TLS_NUM_CONFIG,
+};
+
+static struct proto tls_prots[TLS_NUM_PROTS][TLS_NUM_CONFIG];
int wait_on_pending_writer(struct sock *sk, long *timeo)
{
@@ -342,6 +354,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
struct tls_context *ctx = tls_get_ctx(sk);
struct proto *prot = NULL;
int rc = 0;
+ int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
if (!optval || (optlen < sizeof(*crypto_info))) {
rc = -EINVAL;
@@ -396,7 +409,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
/* currently SW is default, we will have ethtool in future */
rc = tls_set_sw_offload(sk, ctx);
- prot = &tls_sw_prot;
+ prot = &tls_prots[ip_ver][TLS_SW_TX];
if (rc)
goto err_crypto_info;
@@ -443,6 +456,12 @@ static int tls_init(struct sock *sk)
struct inet_connection_sock *icsk = inet_csk(sk);
struct tls_context *ctx;
int rc = 0;
+ int ip_ver = TLSV4;
+
+ if (sk->sk_prot == &tcpv6_prot)
+ ip_ver = TLSV6;
+ else if (sk->sk_prot != &tcp_prot)
+ return -EINVAL;
/* allocate tls context */
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
@@ -453,7 +472,8 @@ static int tls_init(struct sock *sk)
icsk->icsk_ulp_data = ctx;
ctx->setsockopt = sk->sk_prot->setsockopt;
ctx->getsockopt = sk->sk_prot->getsockopt;
- sk->sk_prot = &tls_base_prot;
+
+ sk->sk_prot = &tls_prots[ip_ver][TLS_BASE_TX];
out:
return rc;
}
@@ -464,16 +484,22 @@ static int tls_init(struct sock *sk)
.init = tls_init,
};
+static void build_protos(struct proto *prot, struct proto *base)
+{
+ prot[TLS_BASE_TX] = *base;
+ prot[TLS_BASE_TX].setsockopt = tls_setsockopt;
+ prot[TLS_BASE_TX].getsockopt = tls_getsockopt;
+
+ prot[TLS_SW_TX] = prot[TLS_BASE_TX];
+ prot[TLS_SW_TX].close = tls_sk_proto_close;
+ prot[TLS_SW_TX].sendmsg = tls_sw_sendmsg;
+ prot[TLS_SW_TX].sendpage = tls_sw_sendpage;
+}
+
static int __init tls_register(void)
{
- tls_base_prot = tcp_prot;
- tls_base_prot.setsockopt = tls_setsockopt;
- tls_base_prot.getsockopt = tls_getsockopt;
-
- tls_sw_prot = tls_base_prot;
- tls_sw_prot.sendmsg = tls_sw_sendmsg;
- tls_sw_prot.sendpage = tls_sw_sendpage;
- tls_sw_prot.close = tls_sk_proto_close;
+ build_protos(tls_prots[TLSV4], &tcp_prot);
+ build_protos(tls_prots[TLSV6], &tcpv6_prot);
tcp_register_ulp(&tcp_tls_ulp_ops);
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox