Messages in this thread | | | Subject | Re: flex_array related problems on selinux policy loading | From | Dave Hansen <> | Date | Fri, 21 Jan 2011 07:57:35 -0800 |
| |
On Fri, 2011-01-21 at 08:20 +0100, Steffen Klassert wrote: > On Thu, Jan 20, 2011 at 07:28:50AM -0800, Dave Hansen wrote: > > On Thu, 2011-01-20 at 13:26 +0100, Steffen Klassert wrote: > > ... > > > @@ -187,6 +195,9 @@ int flex_array_put(struct flex_array *fa, unsigned int element_nr, void *src, > > > struct flex_array_part *part; > > > void *dst; > > > > > > + if (unlikely(ZERO_OR_NULL_PTR(fa))) > > > + return 0; > > > > I think it's OK to add these for the array alloc and free cases. But, > > it's really dangerous to do it for put. It has the potential to > > silently throw away data and then be really confusing to debug when you > > can't get it back later. > > If the pointer to struct flex_array is a ZERO_SIZE_PTR we have to exit > before we try to dereference the first time as we have not allocated > anything. We can think about returning an error value in flex_array_put > if the flex_array is a ZERO_SIZE_PTR. The the user would be notified > that we could not store his data, but that's all we can do here I think.
Yeah, a zero-sized array with a put() done on it should return -ENOSPC, not 0. But, an array storing 0-byte objects can and should return 0.
Basically, the patch confuses those two cases.
> > I tend to think about the flex_array itself as being more like a > > kmem_cache than anything else. So, all of the operations on the array > > itself, like shrinking and growing are probably OK. > > Hm, if either element_size or total_nr_elements is zero on allocation time, > the maximum size the array can ever have is zero. So I don't see how to > grow (shrink) anything in this case. Do I miss something here?
I mean it shouldn't return an error, nor is it invalid. You can't _do_ anything, but it's at least valid.
My suggestion would be to simply make sure that the code handles 0-sized objects and 0-length arrays OK, and do it in two separate patches. The ZERO_SIZE_PTR can't be used for both because you need to know which situation you were in and you need different behavior (like in flex_array_put()).
Frankly, I like the idea of just allocating a 'struct flex_array' in any case, and just teaching the code to handle element_size=0 and nr_elements=0. That way, if you have bugs in the code that does things like flex_array_alloc(elem_size=0, len=5, ...) and then flex_array_get(fa, index=99), you have the potential to detect and report the bugs. The only way to do that is to remember what you set the length as.
If you're worried about allocating a whole page, you could easily just kmalloc() a the two integers for the metadata portion of the 'struct flex_array'.
-- Dave
| |