All of lore.kernel.org
 help / color / mirror / Atom feed
From: Breno Leitao <leitao@debian.org>
To: dccp@vger.kernel.org
Subject: Re: [PATCH net-next v4] net: ioctl: Use kernel memory on protocol ioctl callbacks
Date: Fri, 02 Jun 2023 16:19:06 +0000	[thread overview]
Message-ID: <ZHoWeiDVoLV1VMpT@gmail.com> (raw)
In-Reply-To: <20230530175403.2434218-1-leitao@debian.org>

On Wed, May 31, 2023 at 03:01:56AM -0400, Willem de Bruijn wrote:
> Breno Leitao wrote:
> > +int ip6mr_sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg);
> > +static inline int sk_is_ip6mr(struct sock *sk)
> > +{
> > +	return sk->sk_family = AF_INET6 &&
> > +		inet_sk(sk)->inet_num = IPPROTO_ICMPV6;
> > +}
 
> Technically, this is just sk_is_icmpv6, which is broader than IPv6
> multicast routing.

Right, let me rename it to reflect this properly.

> No other concerns from me.

Thanks for the detailed review.

> Two small asides, that are fine to ignore.
> 
> The $PROTO_sk_ioctl functions could conceivably call directly into
> the $PROTO_ioctl functions without the indirect function pointer.
> But that would require open coding the sock_sk_ioctl_inout helpers.
> 
> The demux now first checks relatively unlikely multicast routing
> and phonet before falling through to the more common protocols. But
> ioctl is not a hot path operation.

I am more than happy to open code sock_sk_ioctl_inout into protocol
functions, but, I would prefer to do it in a follow up patch, since this
one is close (I hope) to address the original problem. I hope it works
for you.

Thanks!

WARNING: multiple messages have this Message-ID (diff)
From: Breno Leitao <leitao@debian.org>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: dsahern@kernel.org, Remi Denis-Courmont <courmisch@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Alexander Aring <alex.aring@gmail.com>,
	Stefan Schmidt <stefan@datenfreihafen.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Matthieu Baerts <matthieu.baerts@tessares.net>,
	Mat Martineau <martineau@kernel.org>,
	Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
	Xin Long <lucien.xin@gmail.com>,
	leit@fb.com, axboe@kernel.dk, asml.silence@gmail.com,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	dccp@vger.kernel.org, linux-wpan@vger.kernel.org,
	mptcp@lists.linux.dev, linux-sctp@vger.kernel.org
Subject: Re: [PATCH net-next v4] net: ioctl: Use kernel memory on protocol ioctl callbacks
Date: Fri, 2 Jun 2023 09:19:06 -0700	[thread overview]
Message-ID: <ZHoWeiDVoLV1VMpT@gmail.com> (raw)
In-Reply-To: <6476f0e4b0182_3c8862294b2@willemb.c.googlers.com.notmuch>

On Wed, May 31, 2023 at 03:01:56AM -0400, Willem de Bruijn wrote:
> Breno Leitao wrote:
> > +int ip6mr_sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg);
> > +static inline int sk_is_ip6mr(struct sock *sk)
> > +{
> > +	return sk->sk_family == AF_INET6 &&
> > +		inet_sk(sk)->inet_num == IPPROTO_ICMPV6;
> > +}
 
> Technically, this is just sk_is_icmpv6, which is broader than IPv6
> multicast routing.

Right, let me rename it to reflect this properly.

> No other concerns from me.

Thanks for the detailed review.

> Two small asides, that are fine to ignore.
> 
> The $PROTO_sk_ioctl functions could conceivably call directly into
> the $PROTO_ioctl functions without the indirect function pointer.
> But that would require open coding the sock_sk_ioctl_inout helpers.
> 
> The demux now first checks relatively unlikely multicast routing
> and phonet before falling through to the more common protocols. But
> ioctl is not a hot path operation.

I am more than happy to open code sock_sk_ioctl_inout into protocol
functions, but, I would prefer to do it in a follow up patch, since this
one is close (I hope) to address the original problem. I hope it works
for you.

Thanks!

  parent reply	other threads:[~2023-06-02 16:19 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-30 17:53 [PATCH net-next v4] net: ioctl: Use kernel memory on protocol ioctl callbacks Breno Leitao
2023-05-30 17:53 ` Breno Leitao
2023-05-30 19:38 ` net: ioctl: Use kernel memory on protocol ioctl callbacks: Tests Results MPTCP CI
2023-05-31  7:01 ` [PATCH net-next v4] net: ioctl: Use kernel memory on protocol ioctl callbacks Willem de Bruijn
2023-05-31  7:01   ` Willem de Bruijn
2023-05-31 13:36 ` kernel test robot
2023-05-31 13:36   ` kernel test robot
2023-06-01  3:27 ` kernel test robot
2023-06-01  3:27   ` kernel test robot
2023-06-02 16:19 ` Breno Leitao [this message]
2023-06-02 16:19   ` Breno Leitao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZHoWeiDVoLV1VMpT@gmail.com \
    --to=leitao@debian.org \
    --cc=dccp@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.