On 27/11/17(Mon) 11:49, Mathieu - wrote: > Hi everyone, > > I was looking / poking around the filedesc handling in kern_descrip.c > and found the locking a bit.. weird,
Can you define "weird"? Is it a taste thing or did you find any bug? If it's a bug how can you reproduce it? > especially the fd_getfile function > is touching protected members of the filedesc w/o taking any lock. Which "protected members" are you talking about? What makes you think they are protected? What do you mean with protected? Why do you think they need a lock? > This has been already hit previously in [1] and fixed by reordering the > malloc / free operations in rev 1.138 of kern_descrip.c ([2]) The bug that has been found has been fixed. What are you trying to do? > At the time it was proposed to actually lock fd_getfile which entailed > fixing all the callers. This could be useful for a different reason. Can you guess which one? > I tried to go this way but it proved difficult > as sometimes the locking is done one level higher (e.g namei() being > called with or without the filedesc lock held). > called with or without the filedesc lock held). Introducing a _locked > version of the file was also not feasible for the same reasons (and > anyway it was a bit ugly). Yes it's a complicated task. > So I went down the rabbit hole and tried changing the semantics of the > whole filedesc manipulations in order to make this clearer. Can you define "this"? > [...] > Next step imho would be to get rid of the FIF_LARVAL state. Why? > I know the diff is a bit big and scary because it touches sensitive > paths... but I wanted to have an opinion on it before going further. Before you go any further you need: - to explain what you want to achieve - to have a way to test your changes - to convince us that this is what we want Moving the lock above a level might be something we want. But every time you move the lock/unlock dance to a caller function you might introduce a deadlock. So a big diff is not the way to go. You might also not need to fix all the code paths at once. It depends what you're trying to achieve. > It has been running here for a few days, on a workstation, I also ran > the regress tests w/o any regressions. On which filesystems did you build the system? Which threaded application did you ran? I'd suggest you to split your changes in small reviewable diffs. Make sure you have a goal. Make sure you can test it. You might need to write tests for that.
