dthain / basekernel

A simple OS kernel for research, teaching, and fun.
GNU General Public License v2.0
777 stars 109 forks source link

Process Inheritance creates copy of kobject #248

Closed jmazanec15 closed 5 years ago

jmazanec15 commented 5 years ago

This PR is the first step to address #228 . Originally, in process_selective_inheritance, the parent gave the child process a pointer to each of its kobjects it wanted to inherit. This produced a problem of having undesired shared state across different processes. For example, in window manager, changes to a kobject in two child processes affected each other.

This PR addresses this problem by instead of calling kobject_addref in process_selective_inherit, it calls kobject_copy. Kobject_copy creates a new kobject for the child and creates a shallow copy of of the parent's k_object that is passed in. Note: I am not sure if a new kobject should be initialized inside of the kobject_copy function, or inside of the process_selective_inherit function.

Along with this, I modified how kobject_close and kobject_addref works. I also added a blocking flag for the kobject struct.

Finally, I added a function for each object type called getref that returns the underlying reference count.

dthain commented 5 years ago

Hmm, sorry, I think this is on the wrong track, there is a simpler way to go about it:

1 - We still want reference counting for kobjects, since they can be dup'd within a single process. So kobject_addref and kobject_close should continue to work as before. That allows you to have a single kobject with refcount 2 pointing to (for example) a window with refcount 1.

2 - All kobject_copy needs to do is copy the internal fields, call addref on the underlying object, and return a kobject with refcount 1. That allows you to have two kobjects each referring to the same underlying item.

3 - The getref operations are subtly dangerous: you don't want to allow a caller to make decisions based on the underlying reference count of an object, since that can change between the time you check and the time you act. The only two operations should be to add a reference and to close (potentially destroy) an object.

jmazanec15 commented 5 years ago

The changes you suggested made sense so I just made them. One question on kobject_copy: I still init a new kobject in the function. I do not know if this is the best way to create a new kobject or if it should be created in the process_selective_inherit function.

jmazanec15 commented 5 years ago

I made all of the suggested changes.

I added strdup to string.h, but realized I needed to use kmalloc for the kernel. However, having written strdup, I figured it might be good to have at the user level in the string library.

For copying kobject->tag in kobject copy, I first kmalloc memory for the string and then call strcpy on it. However, this eventually leads to an invalid kfree and I am not sure what is causing it.

dthain commented 5 years ago

Notice that there is a (very) similar string.c in the kernel directory, as well as in the user directory. The trick is to add strdup to the kernel side as well.

dthain commented 5 years ago

And keep in mind that tag may be null, indicating no tag at all, so you have to check for that case too. (Probably the reason for the invalid kfree)

jmazanec15 commented 5 years ago

Those changes made sense so I fixed them. Doing some debugging, the invalid kfree comes from console_delete. I am looking into it now.

jmazanec15 commented 5 years ago

So, the invalid kfree occurs because console_delete tried to kfree the console_root. This has two problems: 1.) console_root is a global variable, which cannot be freed and 2.) the root console is used by the main program so the refcount should not be 0.

To fix 1, I added a check to make sure that the console_root is never freed in console_delete. To fix 2, I added console_addref(&console_root) in main.c.

I am not sure if we want console_root to continue to be a global variable, but this solves the invalid kfree error.

dthain commented 5 years ago

Looks good!