Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   SeongJae Park <sj@kernel.org>
To:     David Hildenbrand <david@redhat.com>
Cc:     "Ryan Roberts" <ryan.roberts@arm.com>,
        "Andrew Morton" <akpm@linux-foundation.org>,
        "Shuah Khan" <shuah@kernel.org>,
        =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse@redhat.com>,
        "Mark Brown" <broonie@kernel.org>,
        "John Hubbard" <jhubbard@nvidia.com>,
        "Florent Revest" <revest@chromium.org>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        "SeongJae Park" <sj@kernel.org>, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission
Date:   Thu, 13 Jul 2023 17:32:37 +0000
Message-Id: <20230713173237.66285-1-sj@kernel.org>
In-Reply-To: <ac816a32-cea9-363f-4490-19e6f5225c3d@redhat.com>
References: 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 13 Jul 2023 16:39:33 +0200 David Hildenbrand <david@redhat.com> wrote:

> On 13.07.23 15:54, Ryan Roberts wrote:
> > When run under run_vmtests.sh, test scripts were failing to run with
> > "permission denied" due to the scripts not being executable.
> > 
> > It is also annoying not to be able to directly invoke run_vmtests.sh,
> > which is solved by giving also it the execute permission.
> > 
> > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> > ---
> >   tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
> >   tools/testing/selftests/mm/check_config.sh             | 0
> >   tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
> >   tools/testing/selftests/mm/run_vmtests.sh              | 0
> >   tools/testing/selftests/mm/test_hmm.sh                 | 0
> >   tools/testing/selftests/mm/test_vmalloc.sh             | 0
> >   tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
> >   tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
> >   8 files changed, 0 insertions(+), 0 deletions(-)
> >   mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh
> > 
> > diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
> > old mode 100644
> > new mode 100755
> 
> Sounds reasonable to me.
> 
> Probably due to:
> 
> commit baa489fabd01596d5426d6e112b34ba5fb59ab82
> Author: SeongJae Park <sj@kernel.org>
> Date:   Tue Jan 3 18:07:53 2023 +0000
> 
>      selftests/vm: rename selftests/vm to selftests/mm
>      
>      Rename selftets/vm to selftests/mm for being more consistent with the
>      code, documentation, and tools directories, and won't be confused with
>      virtual machines.
> 
> 
> and indeed, it contains
> 
> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> old mode 100755
> new mode 100644
> similarity index 100%
> rename from tools/testing/selftests/vm/run_vmtests.sh
> rename to tools/testing/selftests/mm/run_vmtests.sh

Thank you for tracking this and kindly Cc-ing me!  I'd like to clarify a little
bit more, though.  The permission change has made by the commit as you found.
Nevertheless, the submitted version[1] of the patch didn't change the
permission.  I guess the change was made while managing it via some file
permission unsupported patches management tool.

I had a similar issue with DAMON selftest and sent a patch restoring the
permission.  Greg suggested me to update the framework instead, to support such
management tool[2], so I made it[3].  It recently also merged into 5.15.y for
DAMON selftests[4].

I have no strong opinion about whether we need to keep the permission or it's
good to have no execute permission since kselftest framework supports it.  I
just wanted to clarify the events I've shown.  Please correct me if I missed or
wrong something.  Cc-ing Greg, since he might have an opinion.

[1] https://lore.kernel.org/all/20230103180754.129637-5-sj@kernel.org/
[2] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/
[3] https://lore.kernel.org/all/20210810164534.25902-1-sj38.park@gmail.com/
[4] https://lore.kernel.org/stable/2023042743-cheesy-parasitic-206d@gregkh/


Thanks,
SJ

> 
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 
> 
>