netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: 'Christoph Hellwig' <hch@lst.de>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: David Laight <David.Laight@aculab.com>,
	'Christoph Hellwig' <hch@lst.de>,
	Vlad Yasevich <vyasevich@gmail.com>,
	Neil Horman <nhorman@tuxdriver.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	"linux-sctp@vger.kernel.org" <linux-sctp@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: do a single memdup_user in sctp_setsockopt
Date: Sat, 23 May 2020 09:19:29 +0200	[thread overview]
Message-ID: <20200523071929.GA10466@lst.de> (raw)
In-Reply-To: <20200522143623.GA386664@localhost.localdomain>

On Fri, May 22, 2020 at 11:36:23AM -0300, Marcelo Ricardo Leitner wrote:
> It's subjective, yes, but we hardly have patches over 5k lines.
> In the case here, as changing the functions also requires changing
> their call later on the file, it helps to be able to check that is was
> properly updated. Ditto for chained functions.
> 
> For example, I can spot things like this easier (from
> [PATCH 26/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_key)
> 
> @@ -3646,7 +3641,6 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
>         }
> 
>  out:
> -       kzfree(authkey);
>         return ret;
>  }
> ...
> @@ -4771,7 +4765,10 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>         }
> 
>         release_sock(sk);
> -       kfree(kopt);
> +       if (optname == SCTP_AUTH_KEY)
> +               kzfree(kopt);
> +       else
> +               kfree(kopt);
> 
>  out_nounlock:
>         return retval;
> 
> these are 1k lines apart.
> 
> Yet, your implementation around this is better:
> 
> @@ -3733,7 +3624,7 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
>         }
> 
>  out:
> -       kzfree(authkey);
> +       memset(authkey, 0, optlen);
>         return ret;
>  }
> 
> so that sctp_setsockopt() doesn't have to handle it specially.

Actually that implementation is wrong, if you want to move to a plain
kfree it would have to be a memzero_explicit.

> What if you two work on a joint patchset for this? The proposals are
> quite close. The differences around the setsockopt handling are
> minimal already. It is basically variable naming, indentation and one
> or another small change like:

I don't really want to waste too much time on this, as what I really
need is to get the kernel_setsockopt removal series in ASAP.  I'm happy
to respin this once or twice with clear maintainer guidance (like the
memzero_explicit), but I have no idea what you even meant with your
other example or naming.  Tell me what exact changes you want, and
I can do a quick spin, but I don't really want a huge open ended
discussion on how to paint the bikeshed..

Alternatively I'll also happily only do a partial conversion for what
I need for the kernel_setsockopt removal and let you and Dave decided
what you guys prefer for the rest.

  parent reply	other threads:[~2020-05-23  7:19 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 17:46 do a single memdup_user in sctp_setsockopt Christoph Hellwig
2020-05-21 17:46 ` [PATCH 01/49] sctp: copy the optval from user space " Christoph Hellwig
2020-05-21 17:46 ` [PATCH 02/49] sctp: pass a kernel pointer to sctp_setsockopt_bindx Christoph Hellwig
2020-05-21 17:46 ` [PATCH 03/49] sctp: pass a kernel pointer to __sctp_setsockopt_connectx Christoph Hellwig
2020-05-21 17:46 ` [PATCH 04/49] sctp: pass a kernel pointer to sctp_setsockopt_disable_fragments Christoph Hellwig
2020-05-21 17:46 ` [PATCH 05/49] sctp: pass a kernel pointer to sctp_setsockopt_events Christoph Hellwig
2020-05-21 17:46 ` [PATCH 06/49] sctp: pass a kernel pointer to sctp_setsockopt_autoclose Christoph Hellwig
2020-05-21 17:46 ` [PATCH 07/49] sctp: pass a kernel pointer to sctp_setsockopt_peer_addr_params Christoph Hellwig
2020-05-21 17:46 ` [PATCH 08/49] sctp: pass a kernel pointer to sctp_setsockopt_delayed_ack Christoph Hellwig
2020-05-21 17:46 ` [PATCH 09/49] sctp: pass a kernel pointer to sctp_setsockopt_partial_delivery_point Christoph Hellwig
2020-05-21 17:46 ` [PATCH 10/49] sctp: pass a kernel pointer to sctp_setsockopt_initmsg Christoph Hellwig
2020-05-21 17:46 ` [PATCH 11/49] sctp: pass a kernel pointer to sctp_setsockopt_default_send_param Christoph Hellwig
2020-05-21 17:46 ` [PATCH 12/49] sctp: pass a kernel pointer to sctp_setsockopt_default_sndinfo Christoph Hellwig
2020-05-21 17:46 ` [PATCH 13/49] sctp: pass a kernel pointer to sctp_setsockopt_primary_addr Christoph Hellwig
2020-05-21 17:46 ` [PATCH 14/49] sctp: pass a kernel pointer to sctp_setsockopt_peer_primary_addr Christoph Hellwig
2020-05-21 17:46 ` [PATCH 15/49] sctp: pass a kernel pointer to sctp_setsockopt_nodelay Christoph Hellwig
2020-05-21 17:46 ` [PATCH 16/49] sctp: pass a kernel pointer to sctp_setsockopt_rtoinfo Christoph Hellwig
2020-05-21 17:46 ` [PATCH 17/49] sctp: pass a kernel pointer to sctp_setsockopt_associnfo Christoph Hellwig
2020-05-21 17:46 ` [PATCH 18/49] sctp: pass a kernel pointer to sctp_setsockopt_mappedv4 Christoph Hellwig
2020-05-21 17:46 ` [PATCH 19/49] sctp: pass a kernel pointer to sctp_setsockopt_maxseg Christoph Hellwig
2020-05-21 17:46 ` [PATCH 20/49] sctp: pass a kernel pointer to sctp_setsockopt_adaptation_layer Christoph Hellwig
2020-05-21 17:46 ` [PATCH 21/49] sctp: pass a kernel pointer to sctp_setsockopt_context Christoph Hellwig
2020-05-21 17:46 ` [PATCH 22/49] sctp: pass a kernel pointer to sctp_setsockopt_fragment_interleave Christoph Hellwig
2020-05-21 17:46 ` [PATCH 23/49] sctp: pass a kernel pointer to sctp_setsockopt_maxburst Christoph Hellwig
2020-05-21 17:46 ` [PATCH 24/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_chunk Christoph Hellwig
2020-05-21 17:47 ` [PATCH 25/49] sctp: pass a kernel pointer to sctp_setsockopt_hmac_ident Christoph Hellwig
2020-05-21 17:47 ` [PATCH 26/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_key Christoph Hellwig
2020-05-21 17:47 ` [PATCH 27/49] sctp: pass a kernel pointer to sctp_setsockopt_active_key Christoph Hellwig
2020-05-21 17:47 ` [PATCH 28/49] sctp: pass a kernel pointer to sctp_setsockopt_del_key Christoph Hellwig
2020-05-21 17:47 ` [PATCH 29/49] sctp: pass a kernel pointer to sctp_setsockopt_deactivate_key Christoph Hellwig
2020-05-21 17:47 ` [PATCH 30/49] sctp: pass a kernel pointer to sctp_setsockopt_auto_asconf Christoph Hellwig
2020-05-21 17:47 ` [PATCH 31/49] sctp: pass a kernel pointer to sctp_setsockopt_paddr_thresholds Christoph Hellwig
2020-05-21 17:47 ` [PATCH 32/49] sctp: pass a kernel pointer to sctp_setsockopt_recvrcvinfo Christoph Hellwig
2020-05-21 17:47 ` [PATCH 33/49] sctp: pass a kernel pointer to sctp_setsockopt_recvnxtinfo Christoph Hellwig
2020-05-21 17:47 ` [PATCH 34/49] sctp: pass a kernel pointer to sctp_setsockopt_pr_supported Christoph Hellwig
2020-05-21 17:47 ` [PATCH 35/49] sctp: pass a kernel pointer to sctp_setsockopt_default_prinfo Christoph Hellwig
2020-05-21 17:47 ` [PATCH 36/49] sctp: pass a kernel pointer to sctp_setsockopt_reconfig_supported Christoph Hellwig
2020-05-21 17:47 ` [PATCH 37/49] sctp: pass a kernel pointer to sctp_setsockopt_enable_strreset Christoph Hellwig
2020-05-21 17:47 ` [PATCH 38/49] sctp: pass a kernel pointer to sctp_setsockopt_reset_streams Christoph Hellwig
2020-05-21 17:47 ` [PATCH 39/49] sctp: pass a kernel pointer to sctp_setsockopt_reset_assoc Christoph Hellwig
2020-05-21 17:47 ` [PATCH 40/49] sctp: pass a kernel pointer to sctp_setsockopt_add_streams Christoph Hellwig
2020-05-21 17:47 ` [PATCH 41/49] sctp: pass a kernel pointer to sctp_setsockopt_scheduler Christoph Hellwig
2020-05-21 17:47 ` [PATCH 42/49] sctp: pass a kernel pointer to sctp_setsockopt_scheduler_value Christoph Hellwig
2020-05-21 17:47 ` [PATCH 43/49] sctp: pass a kernel pointer to sctp_setsockopt_interleaving_supported Christoph Hellwig
2020-05-21 17:47 ` [PATCH 44/49] sctp: pass a kernel pointer to sctp_setsockopt_reuse_port Christoph Hellwig
2020-05-21 17:47 ` [PATCH 45/49] sctp: pass a kernel pointer to sctp_setsockopt_event Christoph Hellwig
2020-05-21 17:47 ` [PATCH 46/49] " Christoph Hellwig
2020-05-21 17:47 ` [PATCH 47/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_supported Christoph Hellwig
2020-05-21 17:47 ` [PATCH 48/49] sctp: pass a kernel pointer to sctp_setsockopt_ecn_supported Christoph Hellwig
2020-05-21 17:47 ` [PATCH 49/49] sctp: pass a kernel pointer to sctp_setsockopt_pf_expose Christoph Hellwig
2020-05-22  8:02 ` do a single memdup_user in sctp_setsockopt David Laight
2020-05-22 14:36   ` Marcelo Ricardo Leitner
2020-05-22 15:52     ` David Laight
2020-05-23  7:19     ` 'Christoph Hellwig' [this message]
2020-05-25 19:37       ` David Laight
2020-05-25 20:59         ` Marcelo Ricardo Leitner
2020-05-25 21:18       ` Marcelo Ricardo Leitner
2020-05-22 23:11 ` David Miller

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=20200523071929.GA10466@lst.de \
    --to=hch@lst.de \
    --cc=David.Laight@aculab.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=vyasevich@gmail.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).