On 08/05/18(Tue) 14:12, Philip Guenther wrote: > On Tue, 8 May 2018, Martin Pieuchot wrote: > > The way our kernel allocates and populates new 'struct file *' is > > currently a complete mess. One of the problems is that it puts > > incomplete descriptors on the global `filehead' list and on the > > open file array `fd_ofiles'. But to make sure that other threads > > won't use such descriptors before they are ready, they are first > > marked as LARVAL then unmarked via the FILE_SET_MATURE() macro. > > The reason we do that is to meet two goals: > 1) we want to allocate the fd (and fail if we can't) before > possibly blocking while completing the creation of the struct file. > For example, in doaccept(), this thread being blocked waiting for a new > connection must not block other threads from making fd changes.
But the way I understand it, allocating the fd isn't changed by my diff. fdalloc(), called by falloc(), still allocates it and mark the corresponding bits with fd_used(). > 2) close(half_created) and dup2(old, half_created) must be safe: it must > not leak the half_created file in the kernel, and on completion the > half_created fd must either be closed or dup'ed to; the half-created > file must not suddenly show up there later I don't understand, are you talking about close(2) and dup2(2)? How syscalls are related to half_created files? How is that related to my explanation below? > > This is a problem because we'll soon need to serialize accesses > > to the above mentioned data structures with finer locks. And having > > to deal with specific checks in multiple places is simply not viable. > > > > So the diff below changes the logic by introducing a new function: > > fdinsert(). This function will be used to insert new descriptors into > > shared data structured when they are completely initialized. > > doaccept() is still releasing the filedesc lock between falloc() and what > used to be FILE_SET_MATURE() but is now fdinsert(), but that's not safe > since the fd_ofile[] entry is still NULL so finishdup() and close() will > happily overwrite it or return and then later doaccept()->fdinsert() will > overwrite one pointer with another, leaking file references. I admit that doaccept() is the only "syscall" in my diff that releases the fdplock() between falloc() and fdinsert(), however I fail to understand why this isn't safe. Let's see finishdup(). It works with two fd, generally named `old' and `new'. The old one must be MATURE, otherwise fd_getfile() will not find it. To trying to do dup(half_created) or dup2(half_created, new) won't work. The new one is allocated via fdalloc(), which will pick a new free slot. It can't pick the slot of an half_created one because it has been marked by fd_used(). Now finishdup() doesn't call falloc() and it does the same as fdinsert(), so all I see here is a room for improvement :) > So this diff is not safe. I fail to understand why it is not safe. Can you enlighten me?
