Re: [PATCH] Revert "scripts/setlocalversion: git: Make -dirty check more robust"

[Brian Norris <briannorris@chromium.org>]

Hi Genki,

On Tue, Nov 06, 2018 at 11:23:05AM -0800, Genki Sky wrote:
> On Tue,  6 Nov 2018 10:10:38 -0800, Guenter Roeck <linux@roeck-us.net> wrote:
> > This reverts commit 6147b1cf19651c7de297e69108b141fb30aa2349.
> >
> > The reverted patch results in attempted write access to the source
> > repository, even if that repository is mounted read-only.
> >
> > Output from "strace git status -uno --porcelain":
> >
> > getcwd("/tmp/linux-test", 129)          = 16
> > open("/tmp/linux-test/.git/index.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) =
> > 	-1 EROFS (Read-only file system)
> >
> > While git appears to be able to handle this situation, a monitored build
> > environment (such as the one used for Chrome OS kernel builds) may detect
> > it and bail out with an access violation error. On top of that, the attempted
> > write access suggests that git _will_ write to the file even if a build output
> > directory is specified. Users may have the reasonable expectation that the
> > source repository remains untouched in that situation.

I've seen the same problem, by way of working with the same kernel build
system ;)

> Hmm, so in summary: According to 6147b1cf1965
> ("scripts/setlocalversion: git: Make -dirty check more robust",
> 2018-08-28), one scenario requires the index to be refreshed to get a
> correct "dirty" or "not dirty" status. But according to your commit
> here, another scenario requires the kernel build system to not even
> attempt to update the git index, and doesn't care / aren't impacted by
> the cases where the index needs to be refreshed.

I agree with Guenter, that if you're specifying a different build
directory, the source tree should not be written to at all.

> Perhaps both scenarios could be satisfied by having
> scripts/setlocalversion first check if .git has write permissions, and
> acting accordingly. Looking into history, this actually used to be
> done, but cdf2bc632ebc ("scripts/setlocalversion on write-protected
> source tree", 2013-06-14) removed the updating of the index.

A "writeable" check (e.g., [ -w . ]) would be sufficient for our case.
But I'm not so sure about that older NFS report, and I'm also not sure
that we should be writing to the source tree at all in this case. Maybe
we can also check whether there's a build output directory specified?

> However, I admit I don't understand the justification in that commit
> from 2013. I'm no NFS expert, but perhaps the real problem there is an
> incorrectly configured NFS setup (uid/gid mismatch between NFS
> client/server, or permissions mismatch between mount options and NFS
> server?). Christian Kujau: can you speak to that?
> 
> Well, we could also make our check $(touch .git/some-file-here
> 2>/dev/null && ...) instead of $(test -w .git) to handle misconfigured
> NFS setups. But not sure if that has its own problems.

Trying to 'touch' the source tree will also break us. No matter whether
you redirect stderr, our sandbox will still notice the build is doing
something fishy and complain.

In any case, I'd be very happy with a Revert for now (for 4.20, and even
-stable), and a follow-up replacement, so:

Reviewed-by: Brian Norris <briannorris@chromium.org>

for the $subject patch.