Re: [trustees] Custom Trustees: hash element removal and more

[Andrew R. <an...@ae...>]

Noah,

I am CC'ing this to the mailing list so others can be informed about the
recent commits.

On Mon, Feb 12, 2007 at 04:45:56PM -0800, Noah Baker wrote:
> 1. Limited Number of Hash Elements Issue:
>  Solution: Use vmalloc. In order for Trustees to use more than 10000 hash 
> elements, we've switched from using kmalloc/kfree to vmalloc/vfree for hash 
> elements. We've not been able to find any performance hit due to this 
> change. Would you have any reason to caution against this approach? I've 
> included our patch in this email: trustees-vmalloc.patch

The only issue with vmalloc/vfree was locking issues (ensuring that no
locks are held while the vmalloc is performed) other than that, thank
you very much for the suggestion, and I have incorporated the changes to
the subversion.

> 2. Hash Memory Management Without Complete Rebuild Issue:
>  Issue:  Frequent dynamic permission changes caused internal permission 
> lists to grow without bound eventually crashing the server. Due to the user 
> load, it's impractical for us to delete all trustees and fully rebuild the 
> hash everytime a change is needed. We were using the 'clear' permission bit 
> and successive calls to settrustees to achieve this effect. For example:
> [/dev/hda3]/mnt/path:user:CRBXEW  # unset permissions
> [/dev/hda3]/mnt/path:user:RBXE    # set new permissions
> 
> Even though we do a complete hash rebuild nightly, this issue still came up 
> once every 3 or 4 months.
> 
>  Solution: We've added code to settrustees allowing it to send the 
> previously unutilised TRUSTEES_COMMAND_REMOVE command to Trustees, telling 
> it to remove a complete element from the hash (including all permissions 
> associated with the element). Our implementation of the REMOVE command 
> involves prefixing trustee lines with a minus sign. Any users/permissions 
> on the line are ignored (the entire hash element is removed).
> 
> Valid (& equivalent) lines:
> -[/dev/hda3]/mnt/path:user:RBXE
> -[/dev/hda3]/mnt/path:
> 
> Attempting to remove a non-existent hash element makes settrustees bail 
> out, but REMOVEs are handled on the same pass as ADDs, making things like 
> this work as expected:
> 
> [/dev/hda3]/mnt/path:user:W
> -[/dev/hda3]/mnt/path:
> [/dev/hda3]/mnt/path:user:W:user2:R
> -[/dev/hda3]/mnt/path:
> [/dev/hda3]/mnt/path:user:RBXE:user2:R
> 
> Implementing this feature exposed an error in hash reconstruction when 
> removing elements. Even though it seems like free_hash_element sets the 
> hash element's status to DELETED, detailed debug output suggested that this 
> was not taking place. Adding the following in funcs.c fixed hash 
> reconstruction:
> 
> (In case our line numbers don't line up with those currently in svn, this 
> is in the TRUSTEES_COMMAND_REMOVE case block)
> @@ -661,6 +661,7 @@ int trustees_process_command(const struc
> 			goto unlk;
> 		}
> 		free_hash_element(*e);
> +   e->usage = TRUSTEE_HASH_ELEMENT_DELETED;
> 		trustee_hash_deleted++;
> 		free_trustee_name(&name);
> 		r = 0;
> 		
> I've included the patch we use for this functionality: 
> trustees-remove-command.patch
> 
> I'd be interested in hearing your opinion about this remove functionality, 
> and whether or not you think it might be worth including in Trustees in 
> some form. I wonder how and why the REMOVE & REPLACE commands were 
> originally implemented, given that they are not currently used.

I've included some of the functionality to settrustees to not have to
always reparse the file in the case that there are case-insensitive
filesystems so the possibility of having deltaified submissions to the
kernel can be correct on those filesystems.  I'm still debating how to
best handle removing/replacing trustees.  The -[] syntax DOES work
pretty well for something more along the lines of being piped in, but
perhaps it'd be better to just have a:

settrustees --reset-dir [/dev/hda]:/blah

which removes the permissions on [/dev/hda]:/blah and then parses the
config file for any entries that are on that directory and that
directory alone.  

What do you think?  Perhaps some others on the list have some
suggestions for the mechanism to remove trustees on a directory or
manage trustees without having to clear/reset them every time.

> Another note about our in-house Trustees development. I've started work on 
> a test suite for Trustees. It is written in Perl; I call it Testees. 
> Unfortunately, it is quite rudimentary, useful only for testing trustees on 
> a large scale and torture testing. It creates a configurable number of 
> temporary users & n x m directory structures, creates random trustee 
> permission files (you can specify what percentage of available targets to 
> assign trustees to), then verifies permissions on all available targets in 
> its sandbox, based on the applicable trustees. It's useful, but it doesn't 
> do what would be most useful, verifying that Trustees is behaving correctly 
> based on its configuration file. I can't stress enough that the code is 
> debug quality and incomplete, but I'd be happy to send it along if you 
> think it might be useful. I didn't include it in this email because it's 
> somewhat sizable (2000 lines or so).

If you'd be willing to include it in an email to me and were OK with me
adding it to the repository, I'd be happy to include it in the trustees
and hopefully I/you/someone could later add support for making it a full
testsuite.

To everyone else, I've also made several recent commits that overhaul
the settrustees code and the kernel/userspace communication code, my
limited tests show that it works fine, but there may still be issues.
In any case, the hash rebuild bug has been squashed. 

- Andy

-- 
Andrew Ruder <an...@ae...>
http://www.aeruder.net