Skip to content

Commit

Permalink
Fix zfsdev_ioctl() kmem leak warning
Browse files Browse the repository at this point in the history 
Due to an asymmetry in the kmem accounting a memory leak was being
reported when it was only an accounting issue.  All memory allocated
with kmem_alloc() must be released with kmem_free() or it will not
be properly accounted for.

In this case the code used strfree() to release the memory allocated
by kmem_alloc().  Presumably this was done because the size of the
memory region wasn't available when the memory needed to be freed.

To resolve this issue the code has been updated to use strdup() instead
of kmem_alloc() to allocate the memory.  Like strfree(), strdup() is
not integrated with the memory accounting.  This means we can use
strfree() to release it like Illumos.

  SPL: kmem leaked 10/4368729 bytes
  address          size  data             func:line
  ffff880067e9aa40 10    ZZZZZZZZZZ       zfsdev_ioctl:5655

Signed-off-by: Brian Behlendorf <[email protected]>
Signed-off-by: Tim Chase <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes #2262
  • Loading branch information
@behlendorf
behlendorf committed Apr 18, 2014
1 parent e0b8f62 commit 4fd762f
Showing 1 changed file with 9 additions and 5 deletions.
14 changes: 9 additions & 5 deletions module/zfs/zfs_ioctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5584,7 +5584,7 @@ zfsdev_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
{
zfs_cmd_t *zc;
uint_t vecnum;
int error, rc, len = 0, flag = 0;
int error, rc, flag = 0;
const zfs_ioc_vec_t *vec;
char *saved_poolname = NULL;
nvlist_t *innvl = NULL;
Expand Down Expand Up @@ -5651,9 +5651,13 @@ zfsdev_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
goto out;

/* legacy ioctls can modify zc_name */
len = strcspn(zc->zc_name, "/@#") + 1;
saved_poolname = kmem_alloc(len, KM_SLEEP);
(void) strlcpy(saved_poolname, zc->zc_name, len);
saved_poolname = strdup(zc->zc_name);
if (saved_poolname == NULL) {
error = SET_ERROR(ENOMEM);
goto out;
} else {
saved_poolname[strcspn(saved_poolname, "/@#")] = '\0';
}

if (vec->zvec_func != NULL) {
nvlist_t *outnvl;
Expand Down Expand Up @@ -5721,7 +5725,7 @@ zfsdev_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
(void) tsd_set(zfs_allow_log_key, saved_poolname);
} else {
if (saved_poolname != NULL)
kmem_free(saved_poolname, len);
strfree(saved_poolname);
}

kmem_free(zc, sizeof (zfs_cmd_t));
Expand Down

3 comments on commit 4fd762f

@lundman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amusingly, this was causing some issues for us over in OSX. With our slice allocator, often it would allocate from size 16, truncate the string, then spa_strfree() would free it as shorter than 16, and be placed on the size 8 slice. The the memory would be in two slices, and be given out to two allocs simultaneously. Highly amusing. Strangely IllumOS uses kmem_alloc with len to allocate the string, but call spa_strfree (on the shorter string) even though they have len still. We had to go with proper
openzfsonosx/zfs@b042140...e2eafee
but also add debug to our SPL slice allocator to throw a fit when alloc and free sizes do not agree, so we can find these situations much more quickly.

@behlendorf
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this has been a problem in to past as well. I've wondered if we shouldn't just blacklist strdup() and strfree() so they never used. Sadly a a fair bit of code uses them...

@ikozhukhov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with @lundman in his update for osx - i have panic on dilos with current changes too
moved to @lundman version solved panic

Please sign in to comment.