On Tue, Oct 04, 2005 at 04:35:58PM +1000, Kingsley Cheung wrote:
> On Fri, Sep 30, 2005 at 03:58:58PM -0500, Erik Jacobson wrote:
> > Ok, I think I understand now. You're saying since the process never
> > really was running, it doesn't make sense to run __pnotify_exit.
> >
> > In __pnotify_fork, I think I agree with you. If there was a failure
> > forking, the kernel module wouldn't expect to be notified for exit.
> >
> > Also, if the fork callout indicated that the process should fail as a
> > fork failure, I agree, the kernel module wouldn't expect the the
> > exit callout.
> >
> > However, in fork.c's copy_process function, I'm not sure I'm comfortable
> > changing the pnotify_exit call there to pnotify_unsubscribe. I don't want
> > to cause any sort of memory leak where structures a kernel module allocated
> > are not freed.
>
> No, that's definitely not what we want.
>
> >
> > However, we know if our own pnotify_fork call failed, that we don't have
> > anything to cleanup. So I made that jump past the pnotify_exit in the
> > cleanup section. But if anything after pnotify_fork fails, pnotify_exit
> > will be called. This seems pretty normal for copy_process.
> >
> > Does that sound ok?
>
> I'm not sure about that. What I was thinking of was calling
> unsubscribe in pnotify_fork for the process responsible for failing
> the fork. Then the code in copy_process would jump to pnotify_exit
> and cleanup for any other remaining modules that have subscribed.
> This covers all modules and ensures every module that did not veto the
> fork has its exit function called.
>
> >
> > I'll post an updated patch shortly with this and some other feedback.
>
> I'll wait for your updated patch and check it out to see.
>
Erik,
I saw the latest patch you posted. I think there is a bug in
copy_process() with respect to the veto clean up. At the moment the
code is:
retval = pnotify_fork(p, current);
if (retval)
goto bad_fork_cleanup_namespace;
Now if a veto occurs, this would jump over the pnotify_exit() and
never call it. Client modules that may have subscribed successfully
would then not be cleaned up by pnotify_exit(). AFAICS successful
subscriptions are not cleaned up in pnotify_fork() either.
I suppose we could put the cleanup back into pnotify_exit() or we coul
change the above to become:
retval = pnotify_fork(p, current);
if (retval)
goto bad_fork_cleanup_pnotify;
What do you think?
--
Kingsley
|