Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp533688ybg; Wed, 10 Jun 2020 07:12:35 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwMKH3MLDh15sFIvZs8fUZ9tlVEj8o/kPLU14XVvu0Hku3tKWHbaNz5lTnCNhJZO3xPXOb5 X-Received: by 2002:a17:906:7acf:: with SMTP id k15mr3713936ejo.410.1591798355751; Wed, 10 Jun 2020 07:12:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591798355; cv=none; d=google.com; s=arc-20160816; b=fF4z4KT1bNIGr8O/Gmd5RP5k+2bDO29XoEjqfvaGrjuxpXGcYuJtVyF/d8Y6sFhu8m yaJDQPzf6stYgwfyCg3L64WRD4cDS3nz1Fd13aAoJLWrM2FsPL5koj0jeI4oVldzVQSE DjibwGIH91CV+XkNWIrP/PlXzq/JfZrQvMEIEHds9vhoa2P0isn3R1KRe4tB+Ff8Ei00 dkrsNu3+TRY5sGbO+kvBAcn35bIjdLT+D+RYoqYDjGVS8i5O0rygQZ6J5WgMqrz51RvJ wOYxixcKM9X6r1UIahFGvni44TUaO1eQ+UiIvTvalq0wtOuAVpvwhQ2nNkOMd+czUc1c kHpA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=NvTVwV/7ZeXeZVDpWfIR37PrJADcXyFoITN+51THBTY=; b=IaDyOVLqAhWxZAeB8Jb5J8/WeW+DXVBW3piVj8nAEkciVJ4wXl9rLiWYAeEvNGAHS6 Hhdh0NSv9040P8Bs7t0i0VFdFJNbeIU3kaWkbp5UuT5G1hFVjhLSVq2HoqCgX2dip5EU 4kRjFvhwZk8hKXr59bakf10fTgUrrz/+MZMVFHNrUrDlHaZfFg/jBtEXikxxcmrpzKgr xUfB5ibuc5/bvI7iERbqN9/ZRVzM57z71f/Mglc9yu1Me06mg/cAkWT0YRxH22sInk78 /Fp303uWHoES0mFbcumXCJzm91Ut5cARcTH3WIvJHcAwWTBuuN4eOwmT83HZrl/Wj8hA xBMQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id r12si60249eju.597.2020.06.10.07.12.11; Wed, 10 Jun 2020 07:12:35 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727838AbgFJJbF (ORCPT + 99 others); Wed, 10 Jun 2020 05:31:05 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:56045 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726961AbgFJJbE (ORCPT ); Wed, 10 Jun 2020 05:31:04 -0400 Received: from ip5f5af183.dynamic.kabel-deutschland.de ([95.90.241.131] helo=wittgenstein) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jix4R-0005Hy-9B; Wed, 10 Jun 2020 09:30:55 +0000 Date: Wed, 10 Jun 2020 11:30:54 +0200 From: Christian Brauner To: containers@lists.linux-foundation.org, Kees Cook Cc: Giuseppe Scrivano , Robert Sesek , Chris Palmer , Jann Horn , Greg Kroah-Hartman , Daniel Wagner , linux-kernel@vger.kernel.org, Matt Denton , John Fastabend , linux-fsdevel@vger.kernel.org, Tejun Heo , Al Viro , cgroups@vger.kernel.org, stable@vger.kernel.org, "David S . Miller" Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Message-ID: <20200610093054.z7hydwbcmh7ocanw@wittgenstein> References: <20200603011044.7972-1-sargun@sargun.me> <20200603011044.7972-2-sargun@sargun.me> <20200604012452.vh33nufblowuxfed@wittgenstein> <202006031845.F587F85A@keescook> <20200604125226.eztfrpvvuji7cbb2@wittgenstein> <20200605075435.GA3345@ircssh-2.c.rugged-nimbus-611.internal> <202006091235.930519F5B@keescook> <20200609200346.3fthqgfyw3bxat6l@wittgenstein> <202006091346.66B79E07@keescook> <037A305F-B3F8-4CFA-B9F8-CD4C9EF9090B@ubuntu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <037A305F-B3F8-4CFA-B9F8-CD4C9EF9090B@ubuntu.com> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 09, 2020 at 11:27:30PM +0200, Christian Brauner wrote: > On June 9, 2020 10:55:42 PM GMT+02:00, Kees Cook wrote: > >On Tue, Jun 09, 2020 at 10:03:46PM +0200, Christian Brauner wrote: > >> I'm looking at __scm_install_fd() and I wonder what specifically you > >> mean by that? The put_user() seems to be placed such that the install > >> occurrs only if it succeeded. Sure, it only handles a single fd but > >> whatever. Userspace knows that already. Just look at systemd when a > >msg > >> fails: > >> > >> void cmsg_close_all(struct msghdr *mh) { > >> struct cmsghdr *cmsg; > >> > >> assert(mh); > >> > >> CMSG_FOREACH(cmsg, mh) > >> if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type > >== SCM_RIGHTS) > >> close_many((int*) CMSG_DATA(cmsg), > >(cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int)); > >> } > >> > >> The only reasonable scenario for this whole mess I can think of is sm > >like (pseudo code): > >> > >> fd_install_received(int fd, struct file *file) > >> { > >> sock = sock_from_file(fd, &err); > >> if (sock) { > >> sock_update_netprioidx(&sock->sk->sk_cgrp_data); > >> sock_update_classid(&sock->sk->sk_cgrp_data); > >> } > >> > >> fd_install(); > >> } > >> > >> error = 0; > >> fdarray = malloc(fdmax); > >> for (i = 0; i < fdmax; i++) { > >> fdarray[i] = get_unused_fd_flags(o_flags); > >> if (fdarray[i] < 0) { > >> error = -EBADF; > >> break; > >> } > >> > >> error = security_file_receive(file); > >> if (error) > >> break; > >> > >> error = put_user(fd_array[i], ufd); > >> if (error) > >> break; > >> } > >> > >> for (i = 0; i < fdmax; i++) { > >> if (error) { > >> /* ignore errors */ > >> put_user(-EBADF, ufd); /* If this put_user() fails and the first > >one succeeded userspace might now close an fd it didn't intend to. */ > >> put_unused_fd(fdarray[i]); > >> } else { > >> fd_install_received(fdarray[i], file); > >> } > >> } > > > >I see 4 cases of the same code pattern (get_unused_fd_flags(), > >sock_update_*(), fd_install()), one of them has this difficult > >put_user() > >in the middle, and one of them has a potential replace_fd() instead of > >the get_used/fd_install. So, to me, it makes sense to have a helper > >that > >encapsulates the common work that each of those call sites has to do, > >which I keep cringing at all these suggestions that leave portions of > >it > >outside the helper. > > > >If it's too ugly to keep the put_user() in the helper, then we can try > >what was suggested earlier, and just totally rework the failure path > >for > >SCM_RIGHTS. > > > >LOL. And while we were debating this, hch just went and cleaned stuff > >up: > > > >2618d530dd8b ("net/scm: cleanup scm_detach_fds") > > > >So, um, yeah, now my proposal is actually even closer to what we > >already > >have there. We just add the replace_fd() logic to __scm_install_fd() > >and > >we're done with it. > > Cool, you have a link? :) For the record, as I didn't see this yesterday since I was already looking at a kernel with Christoph's changes. His changes just move the logic that was already there before into a separate helper. So effectively nothing has changed semantically in the scm code at all. This is why I was asking yesterday what you meant by reworking the scm code's put_user() logic as it seems obviously correct as it is done now. Christian