Panic in zfs_freebsd_getattr -> zfs_fuid_table_load - avl_find() succeeded inside avl_add() [ACL, 9.1-PRERELEASE] [SOLVED]

[Martin Matuska]

Hi Pawel,

what do you think of the attached patch?
It does the alloc/free more the "illumos" way and also enhances readability.

Cheers,
mm

On 3.9.2012 8:59, Bryan Drewery wrote:
> On 9/3/2012 1:03 AM, Bryan Drewery wrote:
>> On 9/2/2012 11:24 PM, Bryan Drewery wrote:
>>>> On Sep 2, 2012 8:51 PM, "Bryan Drewery" <bryan at shatow.net
>>>> <mailto:bryan at shatow.net>> wrote:
>>>>
>>>>     Running 9.1-PRERELEASE currently.
>>>>
>>>>     Just set this server up, imported the pool from OpenIndiana 151 I
>>>>     believe it was.
>>>>
>>>>     When I access (simply `ls`) certain files/directories, the system
>>>>     panics. These files have ACL properties set on them from the Solaris
>>>>     system.
>>>>
>>>>     This system has 32gb of ram and only 8gb swap setup, so I do not
>>>>     currently have a kernel core dump. It's also practically a production
>>>>     machine, so I do not have much leeway in testing on it.
>>>>
>>>>     backtrace:
>>>>
>>>>     >From running ls(1):
>>>>
>>>>         panic: avl_find() succeeded inside avl_add()
>> I removed my seat belt and made this PANIC into a return; I don't know
>> the impact of this, but I am able to access the files now.
>>
>> I'm looking for what duplicated entries there are. Any advice on this
>> would be appreciated.
>>
>> I'll avoid clearing the ACL properties for now.
> I've solved this and now have a working system.
>
> r230454 [1] fixes this. It had a MFC of 1 week but never made it to
> 9-STABLE.
>
> Please MFC this!
>
> OTOH, the change looks wrong, but I don't know enough to say that for
> certain.
>
>
> Why change kd_name to size 1, and then use strcpy(). Looks like an easy
> overflow.
>
> [1] http://lists.freebsd.org/pipermail/svn-src-head/2012-January/033707.html
>
>
>>
>>>>         avl_add+0x4b
>>>>         zfs_fuid_table_load+0x198
>>>>         zfs_fuid_init+0x12c
>>>>         zfs_fuid_find_by_idx+0xc7
>>>>         zfs_fuid_map_id+0x19
>>>>         zfs_groupmember+0x16
>>>>         zfs_zaccess_aces_check+0x196
>>>>         zfs_zaccess+0xc6
>>>>         zfs_freebsd_getattr+0x1c1
>>>>         vn_stat+0x6a
>>>>         kern_statat_vnhook+0xf9
>>>>         kern_statat+0x15
>>>>         sys_lstat+0x2a
>>>>         amd64_syscall+0x540
>>>>
>>>>     At first I thought this was related to MAC / ugidfw, but I am able to
>>>>     reproduce with those not compiled in. FWIW, here is a backtrace from
>>>>     having that enabled:
>>>>
>>>>         panic: avl_find() succeeded inside avl_add()
>>>>         avl_add+0x4b
>>>>         zfs_fuid_table_load+0x198
>>>>         zfs_fuid_init+0x12c
>>>>         zfs_fuid_find_by_idx+0xc7
>>>>         zfs_fuid_map_id+0x19
>>>>         zfs_groupmember+0x16
>>>>         zfs_zaccess_aces_check+0x196
>>>>         zfs_zaccess+0xc6
>>>>         zfs_freebsd_getattr+0x1c1
>>>>         ugidfw_check_vp+0x6c
>>>>         mac_vnode_check_stat+0xa7
>>>>         vn_stat+0x39
>>>>         kern_statat_vnhook+0xf9
>>>>         kern_statat+0x15
>>>>         sys_stat+0x2a
>>>>         amd64_syscall+0x540
>>>>
>>>>
>>>>     Is there some easy way to clear these ACL properties on the files? I do
>>>>     not need them.
>>>>
>>>>     Any suggestions on how I might fix this or debug this further?
>>>>
>>>>
>>>>     Bryan


-- 
Martin Matuska
FreeBSD committer
http://blog.vx.sk

-------------- next part --------------
Index: sys/cddl/compat/opensolaris/sys/sid.h
===================================================================
--- sys/cddl/compat/opensolaris/sys/sid.h	(revision 239770)
+++ sys/cddl/compat/opensolaris/sys/sid.h	(working copy)
@@ -30,7 +30,8 @@
 #define	_OPENSOLARIS_SYS_SID_H_
 
 typedef struct ksiddomain {
-	char	kd_name[1];	/* Domain part of SID */
+	char	*kd_name;	/* Domain part of SID */
+	uint_t	kd_len;
 } ksiddomain_t;
 typedef void	ksid_t;
 
@@ -38,8 +39,12 @@
 ksid_lookupdomain(const char *domain)
 {
 	ksiddomain_t *kd;
+	size_t len;
 
-	kd = kmem_alloc(sizeof(*kd) + strlen(domain), KM_SLEEP);
+	len = strlen(domain) + 1;
+	kd = kmem_alloc(sizeof(*kd), KM_SLEEP);
+	kd->kd_len = (uint_t)len;
+	kd->kd_name = kmem_alloc(len, KM_SLEEP);
 	strcpy(kd->kd_name, domain);
 	return (kd);
 }
@@ -48,6 +53,7 @@
 ksiddomain_rele(ksiddomain_t *kd)
 {
 
+	kmem_free(kd->kd_name, kd->kd_len);
 	kmem_free(kd, sizeof(*kd));
 }