On Wed, Aug 2, 2023 at 9:11 PM Su Hui <suhui@xxxxxxxxxxxx> wrote:
>
> On 2023/8/3 05:40, Nick Desaulniers wrote:
>
> On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
>
> I noticed that the function in question already has a guard:
> 322 if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
>
> Which implies that hostname COULD be NULL.
>
> Should this perhaps simply be rewritten as:
>
> if (!hostname)
> return NULL;
> if (memchr(...) != NULL)
> ...
>
> Rather than bury yet another guard for the same check further down in
> the function? Check once and bail early.
>
> Hi, Nick Desaulnier,
>
> This may disrupt the logic of this function. So maybe the past one is better.
>
> 322 if (!hostname)
> 323 return NULL;
> ^^^^^^^^^^^^
> If we return in this place.
>
> 324 if (memchr(hostname, '/', hostname_len) != NULL) {
> 325 if (printk_ratelimit()) {
> 326 printk(KERN_WARNING "Invalid hostname \"%.*s\" "
> 327 "in NFS lock request\n",
> 328 (int)hostname_len, hostname);
> 329 }
> 330 return NULL;
> 331 }
> 332
> 333 retry:
> 334 spin_lock(&nsm_lock);
> 335
> 336 if (nsm_use_hostnames && hostname != NULL)
> 337 cached = nsm_lookup_hostname(&ln->nsm_handles,
> 338 hostname, hostname_len);
> 339 else
> 340 cached = nsm_lookup_addr(&ln->nsm_handles, sap);
> ^^^^^^^^^^^^^^^
> This case will be broken when hostname is NULL.
Ah, you're right; I agree.
What are your thoughts then about moving the "is hostname NULL" check
to nsm_create_handle() then?
Perhaps nsm_create_handle() should check that immediately and return
NULL, or simply skip the memcpy if hostname is NULL?
It seems perhaps best to localize this to the callee rather than the
caller. While there is only one caller today, should another arise
someday, they may make the same mistake.
I don't feel strongly either way, and am happy to sign off on either
approach; just triple checking we've considered a few different cases.
>
> 341
> 342 if (cached != NULL) {
> 343 refcount_inc(&cached->sm_count);
> 344 spin_unlock(&nsm_lock);
> 345 kfree(new);
> 346 dprintk("lockd: found nsm_handle for %s (%s), "
> 347 "cnt %d\n", cached->sm_name,
> 348 cached->sm_addrbuf,
> 349 refcount_read(&cached->sm_count));
> 350 return cached;
> 351 }
>
--
Thanks,
~Nick Desaulniers
