[Unionfs] Re: 2.6.26-rc4: smack and unionfs deadlock

[Casey Schaufler]

Luiz Fernando N. Capitulino wrote:
> Em Thu, 05 Jun 2008 22:11:19 -0700
> Casey Schaufler <casey at schaufler-ca.com> escreveu:
>
> | Casey Schaufler wrote:
> | > Luiz Fernando N. Capitulino wrote:
> | >>  Hi Casey,
> | >>
> | >>  I've got another deadlock which only happens when CONFIG_SMACK is
> | >> enabled, but this time it happens with unionfs and I don't know
> | >> which of the two is causing the problem.
> | >>
> | >>   
> | >
> | > I need a little education. unionfs has a lock on its superblock,
> | > which appears to be where we're getting hung up. What condition
> | > is this lock for? It would seem that a getxattr would be pretty
> | > safe without having the superblock locked, but no one is going
> | > to accuse me of being excessively skilled at locking issues.
> | > I'm going to see what goes wrong without that lock, and if removing
> | > it takes care of the problem. There may be more than one dragon
> | > in these waters.
> | >
> | > Thank you.
> | >
> | OK. The sb associated with unionfs has a magic number of 0,
> | which implies you haven't set it. Is this intentional, or
> | do you just figure no one cares, or should care?
> | 
> | I can relieve the hang by having the Smack code treat
> | unionfs (actually the "0" file system) as one of the
> | special cases. The "real" file shows the correct smack
> | value, the unionfs shows "*", and access is correctly
> | controlled in either case. I'm still looking to see what
> | to do better, and I'd still like to understand the locking
> | strategy better.
>
>  Casey, I'm afraid that only Erez (unionfs maintainer)
> can answer you here but it seems that he's away or
> something.
>
>  His last message on unionfs ML dates about one month
> back.
>
>  :(
Ok. Here's a temporary patch that makes everything except looking at
the Smack label work properly. I haven't signed it off because I
don't think it's ready for prime-time, but if the problem is holding
anyone up it will allow them to proceed.

diff -uprN -X linux-2.6.25-g0602-base/Documentation/dontdiff linux-2.6.25-g0602-base/security/smack/smack_lsm.c linux-2.6.25-g0602/security/smack/smack_lsm.c
--- linux-2.6.25-g0602-base/security/smack/smack_lsm.c	2008-06-04 18:18:44.000000000 -0700
+++ linux-2.6.25-g0602/security/smack/smack_lsm.c	2008-06-06 18:15:42.000000000 -0700
@@ -1874,6 +1874,17 @@ static void smack_d_instantiate(struct d
 
 	sbp = inode->i_sb;
 	sbsp = sbp->s_security;
+
+	/*
+	 * If the superblock isn't initialized someone
+	 * has cobbled together a filesystem that never
+	 * got mounted.
+	 */
+	if (sbsp->smk_initialized == 0) {
+		isp->smk_inode = smack_known_star.smk_known;
+		isp->smk_flags |= SMK_INODE_INSTANT;
+		goto unlockandout;
+	}
 	/*
 	 * We're going to use the superblock default label
 	 * if there's no label on the file.
@@ -1913,6 +1924,18 @@ static void smack_d_instantiate(struct d
 		 */
 		final = smack_known_star.smk_known;
 		break;
+	case 0:
+	case UNIONFS_SUPER_MAGIC:
+		/*
+		 * It seems that unionfs uses a fake superblock
+		 * and doesn't set the magic number therein.
+		 * Assume that an unset magic number is unionfs.
+		 *
+		 * Let the real filesystem inodes provide the
+		 * Smack label on access checks.
+		 */
+		final = smack_known_star.smk_known;
+		break;
 	case DEVPTS_SUPER_MAGIC:
 		/*
 		 * devpts seems content with the label of the task.



-- 

----------------------

Casey Schaufler
casey at schaufler-ca.com
650.906.1780