lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Oct 2019 20:24:20 +0200 From: Matteo Croce <mcroce@...hat.com> To: Simon Horman <simon.horman@...ronome.com> Cc: netdev <netdev@...r.kernel.org>, Jay Vosburgh <j.vosburgh@...il.com>, Veaceslav Falico <vfalico@...il.com>, Andy Gospodarek <andy@...yhouse.net>, "David S . Miller" <davem@...emloft.net>, Stanislav Fomichev <sdf@...gle.com>, Daniel Borkmann <daniel@...earbox.net>, Song Liu <songliubraving@...com>, Alexei Starovoitov <ast@...nel.org>, Paul Blakey <paulb@...lanox.com>, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH net-next 3/4] flow_dissector: extract more ICMP information On Fri, Oct 25, 2019 at 8:29 AM Simon Horman <simon.horman@...ronome.com> wrote: > > On Fri, Oct 25, 2019 at 02:27:28AM +0200, Matteo Croce wrote: > > On Wed, Oct 23, 2019 at 7:55 PM Simon Horman <simon.horman@...ronome.com> wrote: > > > > > > On Wed, Oct 23, 2019 at 12:53:37PM +0200, Matteo Croce wrote: > > > > On Wed, Oct 23, 2019 at 12:00 PM Simon Horman > > > > <simon.horman@...ronome.com> wrote: > > > > > On Mon, Oct 21, 2019 at 10:09:47PM +0200, Matteo Croce wrote: > > > > > > + switch (ih->type) { > > > > > > + case ICMP_ECHO: > > > > > > + case ICMP_ECHOREPLY: > > > > > > + case ICMP_TIMESTAMP: > > > > > > + case ICMP_TIMESTAMPREPLY: > > > > > > + case ICMPV6_ECHO_REQUEST: > > > > > > + case ICMPV6_ECHO_REPLY: > > > > > > + /* As we use 0 to signal that the Id field is not present, > > > > > > + * avoid confusion with packets without such field > > > > > > + */ > > > > > > + key_icmp->id = ih->un.echo.id ? : 1; > > > > > > > > > > Its not obvious to me why the kernel should treat id-zero as a special > > > > > value if it is not special on the wire. > > > > > > > > > > Perhaps a caller who needs to know if the id is present can > > > > > check the ICMP type as this code does, say using a helper. > > > > > > > > > > > > > Hi, > > > > > > > > The problem is that the 0-0 Type-Code pair identifies the echo replies. > > > > So instead of adding a bool is_present value I hardcoded the info in > > > > the ID field making it always non null, at the expense of a possible > > > > collision, which is harmless. > > > > > > Sorry, I feel that I'm missing something here. > > > > > > My reading of the code above is that for the cased types above > > > (echo, echo reply, ...) the id is present. Otherwise it is not. > > > My idea would be to put a check for those types in a helper. > > > > > > > Something like icmp_has_id(), I like it. > > > > > I do agree that the override you have used is harmless enough > > > in the context of the only user of the id which appears in > > > the following patch of this series. > > > > > > > > > Some other things I noticed in this patch on a second pass: > > > > > > * I think you can remove the icmp field from struct flow_dissector_key_ports > > > > > > > You mean flow_dissector_key_icmp maybe? > > Yes, sorry for the misinformation. > > > > * I think that adding icmp to struct flow_keys should be accompanied by > > > adding ICMP to flow_keys_dissector_symmetric_keys. But I think this is > > > not desirable outside of the bonding use-case and rather > > > the bonding driver should define its own structures that > > > includes the keys it needs - basically copies of struct flow_keys > > > and flow_keys_dissector_symmetric_keys with some modifications. > > > > > > > Just flow_keys_dissector_symmetric_keys or flow_keys_dissector_keys too? > > Anyway, it seems that the bonding uses the flow_dissector only when > > using encap2+3 or encap3+4 hashing, which means decap some known > > tunnels (mpls and gre and pppoe I think). > > That is the use case I noticed. > > In that case it uses skb_flow_dissect_flow_keys() which in turn > uses struct flow_keys and flow_keys_basic_dissector_keys (which is > assigned to flow_keys_dissector_keys. > > Sorry about mentioning flow_keys_dissector_symmetric_keys, I think > that was a copy-paste-error on my side. > np > In any case, my point is that if you update struct flow_keys then likely > some corresponding change should also be made to one or more of > *__dissector_keys. But such a change would have scope outside of bonding, > which is perhaps undesirable. So it might be better to make local > structures and call __skb_flow_dissect from within the bonding code. > What drawbacks will it have to have the ICMP dissector enabled with flow_keys_dissector_keys? I see three options here: 1. add the ICMP key in flow_keys_dissector_keys and change the flow_dissector behaviour, when dealing with echoes 2. do a local copy in the bonding code 3. leave flow_keys_dissector_keys as is, so the bonding will balance echoes only when not decapping tunnels I don't really know if option 1 could be a bug or a feature, sure option 2 is safer. That can be changed later easily anyway. > > As for other use cases, that do not currently use the dissector, > I think you will need to update them too to get then desired new > feature introduced in patch 4 for those use-cases, which I assume is > desired. Perhaps converting those use-cases to use the flow dissector > is a good way forwards. Perhaps not. > I don't really know why the bonding doesn't use the dissector. Performance? Anyway, maybe converting the bonding to the flow_dissector would make sense, this can be done in the future. I have to talk with the bonding maintainers to understand what's behind this choice. -- Matteo Croce per aspera ad upstream
Powered by blists - more mailing lists