On Thursday 10 April 2008 8:29:19 am Stephen Smalley wrote:
> On Wed, 2008-04-09 at 13:41 -0400, Paul Moore wrote:
> > On Wednesday 09 April 2008 1:08:55 pm Stephen Smalley wrote:
> > > On Mon, 2008-04-07 at 19:11 -0400, Paul Moore wrote:
> > > > diff --git a/security/selinux/netnode.c
> > > > b/security/selinux/netnode.c index f3c526f..b3a5a65 100644
> > > > --- a/security/selinux/netnode.c
> > > > +++ b/security/selinux/netnode.c
> > > > @@ -40,11 +40,17 @@
> > > > #include <net/ipv6.h>
> > > > #include <asm/bug.h>
> > > >
> > > > +#include "netnode.h"
> > > > #include "objsec.h"
> > > >
> > > > #define SEL_NETNODE_HASH_SIZE 256
> > > > #define SEL_NETNODE_HASH_BKT_LIMIT 16
> > > >
> > > > +struct sel_netnode_bkt {
> > > > + u32 size;
> > >
> > > Technically this could just be an ordinary unsigned int, right?
> > > We tend to overuse fixed-size integers in our code at present.
> >
> > [Thanks for taking a look]
> >
> > Yep, we can make this anything we want. I suppose if we are
> > adverse to fixed-size scalars (not really an issue here, but I
> > understand your point) a size_t variable might be a better pick
> > since it is a "size" even though it isn't the traditional memory
> > chunk size. The only drawback is that it varies in size, but it
> > shouldn't messup the packing of the struct.
> >
> > I guess I still kinda like the limit imposed by the u32 (I suppose
> > I could make it smaller, u8?, but the compiler is just going to
> > waste the memory for alignment reasons anyway) but if you feel
> > strongly about this I can change it to an int or size_t type.
>
> Just pointing out that we tend to use fixed-size integers too widely,
> when an ordinary unsigned int or unsigned long would suffice. We
> only generally need the fixed size integers for file (e.g. policy
> image) and network packet formats. Not new to this code, but
> something to be aware of.
I switched it over to an unsigned int in the "v2" revision I posted last
night. You're right in that there is no _need_ for it to be a fixed
signed integer so we might as well let it be a more "native" type.
> > > > @@ -179,35 +183,22 @@ static int sel_netnode_insert(struct
> > > > sel_netnode *node) default:
> > > > BUG();
> > > > }
> > > > - list_add_rcu(&node->list, &sel_netnode_hash[idx]);
> > > >
> > > > /* we need to impose a limit on the growth of the hash table
> > > > so check * this bucket to make sure it is within the specified
> > > > bounds */ - list_for_each_entry(iter, &sel_netnode_hash[idx],
> > > > list) - if (++count > SEL_NETNODE_HASH_BKT_LIMIT) {
> > > > - list_del_rcu(&iter->list);
> > > > - call_rcu(&iter->rcu, sel_netnode_free);
> > > > - break;
> > > > - }
> > > > + list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
> > > > + if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT)
> > > > { + struct sel_netnode *tail;
> > > > + tail = list_entry(node->list.prev, struct sel_netnode,
> > > > list); + __list_del(node->list.prev, node->list.next);
> > >
> > > Can you clarify the change from list_del_rcu() to __list_del()
> > > here?
> >
> > Well, the main reason for the change is I want to delete the tail
> > of the list without having to traverse the entire list, there
> > doesn't appear to be a list_*_rcu() that will allow me to do this
> > so I simply used the __list_del() function to do what I needed.
> > The only danger here is maintaining all of the right RCUisms and
> > looking at the different implementations the only real difference
> > is that the RCU version sets the removed entry's prev pointer to a
> > poison value. Since I "immediately" remove the entry and don't
> > make use of the prev pointer in any of the list functions I decided
> > to just skip that step.
> >
> > However, I am not an RCU expert (tricky thing to master) so if this
> > sounds problematic to you let me know and I'll add the "poisoning"
> > to the code.
>
> I don't quite understand. {snip}
Yeah, now I guess I don't either. For some reason when I wrote this
code I thought I needed to use a __list_del() call to remove the last
entry in the list when in reality I can do the same thing with
list_del_rcu(). No idea what I was thinking then ... give me a few
hours and I'll resubmit.
Thanks.
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
