On Thu, 2023-08-03 at 09:38 -0700, Nick Desaulniers wrote:
> 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.
>
I think that sounds like the best fix here. Just have nsm_create_handle
return NULL immediately if hostname is NULL.
> >
> > 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 }
> >
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
