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
| ||
|
Message-ID: <CAF=yD-+ZwVv7RgNoRzCbP0S6rLPqtjwTsXUs07p_WZqvdx=VnQ@mail.gmail.com> Date: Wed, 23 Jan 2019 09:11:30 -0500 From: Willem de Bruijn <willemdebruijn.kernel@...il.com> To: Maxim Mikityanskiy <maximmi@...lanox.com> Cc: "David S. Miller" <davem@...emloft.net>, Saeed Mahameed <saeedm@...lanox.com>, Willem de Bruijn <willemb@...gle.com>, Jason Wang <jasowang@...hat.com>, Eric Dumazet <edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Eran Ben Elisha <eranbe@...lanox.com>, Tariq Toukan <tariqt@...lanox.com> Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value On Wed, Jan 23, 2019 at 5:17 AM Maxim Mikityanskiy <maximmi@...lanox.com> wrote: > > > -----Original Message----- > > From: Willem de Bruijn <willemdebruijn.kernel@...il.com> > > Sent: 17 January, 2019 17:16 > > To: Maxim Mikityanskiy <maximmi@...lanox.com> > > Cc: David S. Miller <davem@...emloft.net>; Saeed Mahameed > > <saeedm@...lanox.com>; Willem de Bruijn <willemb@...gle.com>; Jason Wang > > <jasowang@...hat.com>; Eric Dumazet <edumazet@...gle.com>; > > netdev@...r.kernel.org; Eran Ben Elisha <eranbe@...lanox.com>; Tariq Toukan > > <tariqt@...lanox.com> > > Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value > > > > On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@...lanox.com> > > wrote: > > > > > > > This is a lot of code change. This would do. > > > > > > > > @@ -2434,8 +2434,6 @@ static inline void > > > > skb_probe_transport_header(struct sk_buff *skb, > > > > > > > > if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, > > 0)) > > > > skb_set_transport_header(skb, keys.control.thoff); > > > > - else > > > > - skb_set_transport_header(skb, offset_hint); > > > > } > > > > > > > > Though leaving an unused argument is a bit ugly. For net-next, indeed > > > > better to clean up (please mark your patchset with net or net-next, > > > > btw) > > > > > > It's for net-next (I'll resend with the correct mark), so I'll stick > > > with the current implementation. > > > > Absolutely, sounds good. > > > > > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen- > > > > netback/netback.c > > > > > index 80aae3a32c2a..b49b6e56ca47 100644 > > > > > --- a/drivers/net/xen-netback/netback.c > > > > > +++ b/drivers/net/xen-netback/netback.c > > > > > @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue > > > > *queue) > > > > > struct xen_netif_tx_request *txp; > > > > > u16 pending_idx; > > > > > unsigned data_len; > > > > > + bool th_set; > > > > > > > > > > pending_idx = XENVIF_TX_CB(skb)->pending_idx; > > > > > txp = &queue->pending_tx_info[pending_idx].req; > > > > > @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct > > xenvif_queue > > > > *queue) > > > > > continue; > > > > > } > > > > > > > > > > - skb_probe_transport_header(skb, 0); > > > > > + th_set = skb_try_probe_transport_header(skb); > > > > > > > > Can use skb_transport_header_was_set(). Then at least there is no need > > > > to change the function's return value. > > > > > > I suppose this comment relates to the previous one, and if we do it for > > > net-next, it's fine to make change I made, isn't it? > > > > If this is the only reason for the boolean return value, using > > skb_transport_header_was_set() is more standard (I immediately know > > what's happening when I read it), slightly less code change and avoids > > introducing a situation where the majority of callers ignore a return > > value. I think it's preferable. But these merits are certainly > > debatable, so either is fine. > > From my side, I wanted to avoid calling skb_transport_header_was_set > twice, so I made skb_try_probe_transport_header return whether it > succeeded or not. I think "try" in the function name indicates this idea > pretty clearly. This result status is pretty useful, it just happened > that it's not needed in many places, Which is an indication that it's perhaps not needed. > but the general idea is that we > report this status, so if you say that my version is also good for you, > I'll leave it as is. It was just a rationale for my decision. It's fine. But please avoid the code churn in xenvif_tx_submit with to extra indentation. This is equivalent: - if (skb_is_gso(skb)) { + if (skb_is_gso(skb) && th_set) { More fundamentally, the code has the assumption that th_set always holds if skb_is_gso(skb). Why add the check? This is another example that the return value is not really needed.
Powered by blists - more mailing lists