Re: [PATCH v3 26/26] objtool: Add STT_NOTYPE noinstr validation

[Julien Thierry]

On 3/25/20 4:50 PM, Peter Zijlstra wrote:
> On Wed, Mar 25, 2020 at 11:40:46AM -0500, Josh Poimboeuf wrote:
> > On Wed, Mar 25, 2020 at 04:53:48PM +0100, Peter Zijlstra wrote:
> > > On Wed, Mar 25, 2020 at 09:42:11AM -0500, Josh Poimboeuf wrote:
> > > > Sure, but couldn't validate_unwind_hints() and
> > > > validate_reachable_instructions() be changed to *only* run on
> > > > .noinstr.text, for the vmlinux case?  That might help converge the
> > > > vmlinux and !vmlinux paths.
> > >
> > > You're thinking something like so then?
> >
> > Not exactly.  But I don't want to keep churning this patch set.  I can
> > add more patches later, so don't worry about it.
> >
> > But I was thinking it would eventually be good to have the top-level
> > check() be like
> >
> > 	sec = NULL;
> > 	if (!validate_all)
> > 		sec = find_section_by_name(file->elf, ".noinstr.text");
> >
> > 	ret = validate_functions(&file, sec);
> > 	if (ret < 0)
> > 		goto out;
> > 	warnings += ret;
> >
> > 	ret = validate_unwind_hints(&file, sec);
> > 	if (ret < 0)
> > 		goto out;
> > 	warnings += ret;
> >
> > 	if (!warnings) {
> > 		ret = validate_reachable_instructions(&file, sec);
> > 		if (ret < 0)
> > 			goto out;
> > 		warnings += ret;
> > 	}
> >
> > 	if (!validate_all)
> > 		goto out;
> >
> > 	ret = validate_retpoline(&file);
> > 	....
> >
> > that way the general flow is the same regardless...
>
> Ah,... I see what you mean, there's two little wrinkles with that:
>
>   - validate_reachable_instructions() is strictly superluous, it does no
>     additional validation between the !vmlinux and vmlinux mode, so I'd
>     put that if (!validate_all) goto out, one up.
>
>   - tglx has a patch adding .entry.text to be considered as noinstr as a
>     whole, which has:
>
>
> @@ -2636,11 +2637,16 @@ static int validate_vmlinux_functions(st
>   	int warnings = 0;
>   
>   	sec = find_section_by_name(file->elf, ".noinstr.text");
> -	if (!sec)
> -		return 0;
> +	if (sec) {
> +		warnings += validate_section(file, sec);
> +		warnings += validate_unwind_hints(file, sec);
> +	}
>   
> -	warnings += validate_section(file, sec);
> -	warnings += validate_unwind_hints(file, sec);
> +	sec = find_section_by_name(file->elf, ".entry.text");
> +	if (sec) {
> +		warnings += validate_section(file, sec);
> +		warnings += validate_unwind_hints(file, sec);
> +	}
>   
>   	return warnings;
>   }
>
> Anyway, yes, we can do this on top.
>
> I was going to commit the first 17 patches to tip/core/objtool and
> repost the remaining 13 once more. And then see how much pain I did to
> Julien's patches :-)

I appreciate the concideration for my patches but, since your patches 
have been posted a few times already and were already reviewed, you 
might as well commit them. I'll rebase my patches on top and see the 
state of things (I'll need to do that with the whole arm64 series anyway).

I'll try to post the new version fast enough so I'm not behind some 
other major objtool changes :) .

In the mean time I'll have a look at this series and see what I might 
have to change for the rest of the arm64-objtool set!

Thanks,

--
Julien Thierry