Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   KP Singh <kpsingh@chromium.org>
Date:   Fri, 27 Mar 2020 16:06:37 +0100
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        Brendan Jackman <jackmanb@google.com>,
        Florent Revest <revest@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        James Morris <jmorris@namei.org>,
        Kees Cook <keescook@chromium.org>,
        Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH bpf-next v7 4/8] bpf: lsm: Implement attach, detach and
 execution
Message-ID: <20200327150637.GA23032@chromium.org>
References: <20200326142823.26277-1-kpsingh@chromium.org>
 <20200326142823.26277-5-kpsingh@chromium.org>
 <20200327031256.vhk2luomxgex3ui4@ast-mbp>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200327031256.vhk2luomxgex3ui4@ast-mbp>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 26-M?r 20:12, Alexei Starovoitov wrote:
> On Thu, Mar 26, 2020 at 03:28:19PM +0100, KP Singh wrote:
> >  
> >  	if (arg == nr_args) {
> > -		if (prog->expected_attach_type == BPF_TRACE_FEXIT) {
> > +		/* BPF_LSM_MAC programs only have int and void functions they
> > +		 * can be attached to. When they are attached to a void function
> > +		 * they result in the creation of an FEXIT trampoline and when
> > +		 * to a function that returns an int, a MODIFY_RETURN
> > +		 * trampoline.
> > +		 */
> > +		if (prog->expected_attach_type == BPF_TRACE_FEXIT ||
> > +		    prog->expected_attach_type == BPF_LSM_MAC) {
> >  			if (!t)
> >  				return true;
> >  			t = btf_type_by_id(btf, t->type);
> 
> Could you add a comment here that though BPF_MODIFY_RETURN-like check
> if (ret_type != 'int') return -EINVAL;
> is _not_ done here. It is still safe, since LSM hooks have only
> void and int return types.

Good idea, I reworded the comment to make this explicit and moved
the comment to inside the if condition.

> 
> > +	case BPF_LSM_MAC:
> > +		if (!prog->aux->attach_func_proto->type)
> > +			/* The function returns void, we cannot modify its
> > +			 * return value.
> > +			 */
> > +			return BPF_TRAMP_FEXIT;
> > +		else
> > +			return BPF_TRAMP_MODIFY_RETURN;
> 
> I was thinking whether it would help performance significantly enough
> if we add a flavor of BPF_TRAMP_FEXIT that doesn't have
> BPF_TRAMP_F_CALL_ORIG.

Agreed.

> That will save the cost of nop call, but I guess indirect call due
> to lsm infra is slow enough, so this extra few cycles won't be noticeable.
> So I'm fine with it as-is. When lsm hooks will get rid of indirect call
> we can optimize it further.

Also agreed, that's the next step. :)

- KP