From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 05903C433E0 for ; Tue, 30 Jun 2020 02:19:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AE4852078B for ; Tue, 30 Jun 2020 02:19:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="jyHvLS0P" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727024AbgF3CTP (ORCPT ); Mon, 29 Jun 2020 22:19:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54066 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725988AbgF3CTP (ORCPT ); Mon, 29 Jun 2020 22:19:15 -0400 Received: from mail-qt1-x843.google.com (mail-qt1-x843.google.com [IPv6:2607:f8b0:4864:20::843]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 055EFC061755 for ; Mon, 29 Jun 2020 19:19:15 -0700 (PDT) Received: by mail-qt1-x843.google.com with SMTP id e12so14472681qtr.9 for ; Mon, 29 Jun 2020 19:19:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rtGevn5rgGHhDa4eV3zCSsDCKQR+raUIp4lYg2I37J4=; b=jyHvLS0PmaN21fjbETeyn/yPy5eoACCRmwRaemWQq4xVOJcq3MVyQM9U6iWV/uLEFW SO6N2crhWPpduYhenwhtd1/exsXWT7Zzf2wgHlJ7BRqL3CRu4sDiAEn0QhYBFpFiy4sz 7zu+zx2u2Lyddj4lIa2nK5z9D+PT+mZaWpeLNU0fSOc5vzOMBqpiGg9WsYbNQZWLZc0a IQ/Yq7YtkA2VKoKjqRFj9yCgkjkQqFXbd5CVdRo/lDWlFs06HJGMwRmL8m1jYusO8K9T 7r2RpVU3oNwlHeK1t2nJfgy/A9W5QtJjpIzNSJlDiBcZdJD5hBuL7yvCMrYKqrs8yQfi 1ZGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rtGevn5rgGHhDa4eV3zCSsDCKQR+raUIp4lYg2I37J4=; b=NpV+Z6/rjBt5SVWawto8UloPhNhs30SReodabPxsCUClUWz5RKmHGbY8oVRA+okNfr Bavl/n+OU1BJ1tJR30+TlDwNcHuRqdX3uRS3UFq/dYCVbmWDxqvIGv4Lxc9saBBsc43w /tibz8hLWvswXncofCsUDtCQAz2fyhAS5ocdlMnNQOOXwmOBeLwIpV0Ugp/fXE9RZ6mO SXJ/WiplKnrCYioi1Q6wVJsD31jy4aVDsIjHwnz/P9HMazmUuedCNfUx1HdrnKqjHchG sbfBKbuimevD5g+RlDQa7NPN5BWtOvIMmcczqnO7b3i1XIoJDQxNTFL4xWIiY/n2LnaT aN9w== X-Gm-Message-State: AOAM531X3T0QjPtHnGk2P1AgWhyFGMCKAHIPwtunpAqTdlsxbxaviNDX /hf9B0RuEvab1f4QZUC9jSVLjwYT X-Google-Smtp-Source: ABdhPJxwSsxCQulXU00d83R0guw/BPS+njgnVrZFh62Ko4/hYBqfBodVuDlcW1DhddVOZtZbiIxwAw== X-Received: by 2002:ac8:f8b:: with SMTP id b11mr19087194qtk.361.1593483553447; Mon, 29 Jun 2020 19:19:13 -0700 (PDT) Received: from mail-yb1-f172.google.com (mail-yb1-f172.google.com. [209.85.219.172]) by smtp.gmail.com with ESMTPSA id p7sm1814677qki.61.2020.06.29.19.19.12 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 29 Jun 2020 19:19:12 -0700 (PDT) Received: by mail-yb1-f172.google.com with SMTP id j19so6321075ybj.1 for ; Mon, 29 Jun 2020 19:19:12 -0700 (PDT) X-Received: by 2002:a25:be02:: with SMTP id h2mr30746440ybk.315.1593483551816; Mon, 29 Jun 2020 19:19:11 -0700 (PDT) MIME-Version: 1.0 References: <20200629165731.1553050-1-willemdebruijn.kernel@gmail.com> In-Reply-To: From: Willem de Bruijn Date: Mon, 29 Jun 2020 22:18:34 -0400 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH net-next] icmp: support rfc 4884 To: Tom Herbert Cc: Eric Dumazet , Willem de Bruijn , Network Development , David Miller Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Jun 29, 2020 at 8:37 PM Tom Herbert wrote: > > On Mon, Jun 29, 2020 at 4:07 PM Eric Dumazet wrote: > > > > > > > > On 6/29/20 2:30 PM, Willem de Bruijn wrote: > > > On Mon, Jun 29, 2020 at 5:15 PM Eric Dumazet wrote: > > >> > > >> > > >> > > >> On 6/29/20 9:57 AM, Willem de Bruijn wrote: > > >>> From: Willem de Bruijn > > >>> > > >>> ICMP messages may include an extension structure after the original > > >>> datagram. RFC 4884 standardized this behavior. > > >>> > > >>> It introduces an explicit original datagram length field in the ICMP > > >>> header to delineate the original datagram from the extension struct. > > >>> > > >>> Return this field when reading an ICMP error from the error queue. > > >> > > >> RFC mentions a 'length' field of 8 bits, your patch chose to export the whole > > >> second word of icmp header. > > >> > > >> Why is this field mapped to a prior one (icmp_hdr(skb)->un.gateway) ? > > >> > > >> Should we add an element in the union to make this a little bit more explicit/readable ? > > >> > > >> diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h > > >> index 5589eeb791ca580bb182e1dc38c05eab1c75adb9..427ed5a6765316a4c1e2fa06f3b6618447c01564 100644 > > >> --- a/include/uapi/linux/icmp.h > > >> +++ b/include/uapi/linux/icmp.h > > >> @@ -76,6 +76,7 @@ struct icmphdr { > > >> __be16 sequence; > > >> } echo; > > >> __be32 gateway; > > >> + __be32 second_word; /* RFC 4884 4.[123] : ,, */ > > >> struct { > > >> __be16 __unused; > > >> __be16 mtu; > > > > > > Okay. How about a variant of the existing struct frag? > > > > > > @@ -80,6 +80,11 @@ struct icmphdr { > > > __be16 __unused; > > > __be16 mtu; > > > } frag; > > > + struct { > > > + __u8 __unused; > > > + __u8 length; > > > + __be16 mtu; > > > + } rfc_4884; > > > __u8 reserved[4]; > > > } un; > > > > > > > Sure, but my point was later in the code : > > > > >>> + if (inet_sk(sk)->recverr_rfc4884) > > >>> + info = ntohl(icmp_hdr(skb)->un.gateway); > > >> > > >> ntohl(icmp_hdr(skb)->un.second_word); > > > > If you leave there "info = ntohl(icmp_hdr(skb)->un.gateway)" it is a bit hard for someone > > reading linux kernel code to understand why we do this. > > > It's also potentially problematic. The other bits are Unused, which > isn't the same thing as necessarily being zero. Userspace might assume > that info is the length without checking its bounded. It shouldn't. The icmp type and code are passed in sock_extended_err as ee_type and ee_code. So it can demultiplex the meaning of the rest of the icmp header. It just needs access to the other 32-bits, which indeed are context sensitive. It makes more sense to me to let userspace demultiplex this in one place, rather than demultiplex in the kernel and define a new, likely no simpler, data structure to share with userspace. Specific to RFC 4884, the 8-bit length field coexists with the 16-bit mtu field in case of ICMP_FRAG_NEEDED, so we cannot just pass the first as ee_info in RFC 4884 mode. sock_extended_err additionally has ee_data, but after that we're out of fields, too, so this approach is not very future proof to additional ICMP extensions. On your previous point, it might be useful to define struct rfc_4884 equivalent outside struct icmphdr, so that an application can easily cast to that. RFC 4884 itself does not define any extension objects. That is out of scope there, and in my opinion, here. Again, better left to userspace. Especially because as it describes, it standardized the behavior after observing non-compliant, but existing in the wild, proprietary extension variants. Users may have to change how they interpret the fields based on what they have deployed.