Re: Setting security XATTR on ubifs

[Artem Bityutskiy <dedekind1@gmail.com>]

Hi,

no time now, so very quick answer.

On Fri, 2011-05-20 at 16:36 -0700, Subodh Nijsure wrote:
> we have implemented modifications to UBIFS to add support for SELinux
> labeling. Function that created this XATTR is called
> ubifs_init_security(), shown below.

OK, sounds cool.

> Following example of how JFFS2 does extended attribute labeling, This
> function is being called from 
> ubifs_create(),ubifs_mkdir(), ubifs_mknod(), ubifs_symlink() (in
> fs/ubifs/dir.c)

OK.

> With this modification things work "mostly", I am able to label the file
> system, but sometimes the file system is getting corrupted. I will
> certainly post the patch once things work reliably. 

Hmm... Running tests with all UBIFS extra self-checks turned on is a
good idea, BTW.

> I don't _fully_ understand how ubifs is doing space management, hence
> the immediate questions I have are:

Well, the main complication is budgeting: before doing any I/O you need
to get space budget, which is basically about asking the budgeting
subsystem to reserve certain amount of space for your operation. When
you are done - you release the budget.

> 1. What is the right point to add the XATTR to the UBIFS inode, after
> the ubifs_new_inode() is done?

I guess right after that? But keep in mind that unclean reboots may
result in losing selinux attributes. E.g., you create a new inode for
file XXX and this succeeds, then you start creating the selinux labels
and you have a power cut. When you mount the FS again you'll find out
that XXX does not have any selinux labels.

I do not know selinux well enough so do not know how this is handled:
does selinux handle it from user-space somehow or it assumes that
file/dir/symlink + selinux lables creation is atomic?

>  Should ubifs_budget_space() be updated to
> handle extra space needed by the XATTR.

In _your_ implementation no - ubifs_init_security() calls
'ubifs_setxattr()' which calls 'create_xattr()' which gets the budget.

I mean, in your implementation creating inode and creating selinux
xattrs are 2 separate operations and you use existing "big" building
blocks for this which do the budgeting.

> 2. In function below ui_mutex is being locked/unlocked while XATTR for
> the file is updated. Is that required while updating the extended
> attribute?

You'd deadlock if you did this I think, because 'create_xattr()' takes
the mutex (it is called "host_ui->ui_mutex" there).

HTH.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)