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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH] hfsplus: Fix bug causing custom uid and gid being unable
 to be assigned with mount
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <CD80923F-4422-416B-AAC1-A676D37C564F@live.com>
Date:   Tue, 6 Dec 2022 10:56:56 -0800
Cc:     "willy@infradead.org" <willy@infradead.org>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "songmuchun@bytedance.com" <songmuchun@bytedance.com>,
        "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <20EA622D-12B9-49B6-8564-C8EE2A2329D2@dubeyko.com>
References: <93F5197D-2B61-4129-B5D4-771934F70577@live.com>
 <55A80630-60FB-44BE-9628-29104AB8A7D0@dubeyko.com>
 <1D7AAEE4-9603-43A4-B89D-6F791EDCB929@live.com>
 <A2B962C1-AD33-413D-B64A-CD179AFBEA8D@dubeyko.com>
 <FBF299DB-E235-4BD3-82BD-5A54EE9E26DA@live.com>
 <CD80923F-4422-416B-AAC1-A676D37C564F@live.com>
To:     Aditya Garg <gargaditya08@live.com>
Precedence: bulk


> On Dec 6, 2022, at 12:49 AM, Aditya Garg <gargaditya08@live.com> =
wrote:
>=20
>>=20
>> Well initially I when I tried to investigate what=E2=80=99s wrong, =
and found that the old logic was the culprit, I did some logging to see =
what exactly was wrong. The log patch is here btw :- =
https://github.com/AdityaGarg8/linux/commit/f668fb012f595d83053020b88b9439=
c295b4dc21
>>=20
>> So I saw that the old logic was always false, no matter whether I =
mounted with uid or not.
>>=20
>> I tried to see what makes this true, but couldn't succeed. So, I =
thought of a simpler approach and changed the logic itself.
>>=20
>> To be honest, I dunno what is the old logic for. Maybe instead of =
completely removing the old logic, I could use an OR?
>>=20
>> If you think its more logical, I can make this change :-
>>=20
>> -	if (!i_gid_read(inode) && !mode)
>> +	if ((test_bit(HFSPLUS_SB_UID, &sbi->flags)) || =
(!i_uid_read(inode) && !mode))
>>=20
>> Thanks
>> Aditya
>>=20
>>=20
>=20
> I continuation with this message, I also think the bits should be set =
only if (!uid_valid(sbi->uid) is false, else the bits may be set even if =
UID is invalid? So, do you think the change given below should be good =
for this?
>=20
> diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
> index 047e05c57..c94a58762 100644
> --- a/fs/hfsplus/options.c
> +++ b/fs/hfsplus/options.c
> @@ -140,6 +140,8 @@ int hfsplus_parse_options(char *input, struct =
hfsplus_sb_info *sbi)
> 			if (!uid_valid(sbi->uid)) {
> 				pr_err("invalid uid specified\n");
> 				return 0;
> +			} else {
> +				set_bit(HFSPLUS_SB_UID, &sbi->flags);
> 			}
> 			break;
> 		case opt_gid:
> @@ -151,6 +153,8 @@ int hfsplus_parse_options(char *input, struct =
hfsplus_sb_info *sbi)
> 			if (!gid_valid(sbi->gid)) {
> 				pr_err("invalid gid specified\n");
> 				return 0;
> +			} else {
> +				set_bit(HFSPLUS_SB_GID, &sbi->flags);
> 			}
> 			break;
> 		case opt_part:

Looks reasonably well. I believe it=E2=80=99s better fix.

Thanks,
Slava.