Re: [PATCH 3/4] coredump: ELF-FDPIC: enable to omit anonymous shared memory

["Kawai, Hidehiro" <hidehiro.kawai.ez@hitachi.com>]

Hi,

Thank you for your reply.

David Howells wrote:

>>I think that locking makes codes complex and generates overhead.
>>So I wouldn't like to use lock as far as possible.  I think passing
>>the flag as an extra argument is the simplest implementation to
>>avoid the core file corruption.
> 
> Actually, I don't think the locking is that hard or that complex.
> 
> 	int do_coredump(long signr, int exit_code, struct pt_regs * regs)
> 	{
> 		<setup vars>
> 
> 		down_read(&coredump_settings_sem);
> 
> 		...
> 
> 	fail:
> 		up_read(&coredump_settings_sem);
> 		return retval;
> 	}
> 
> And:
> 
> 	static ssize_t proc_coredump_omit_anon_shared_write(struct file *file,
> 						    const char __user *buf,
> 						    size_t count,
> 						    loff_t *ppos)
> 	{
> 		<setup vars>
> 
> 		down_write(&coredump_settings_sem);
> 
> 		...
> 
> 	out_no_task:
> 		up_write(&coredump_settings_sem);
> 		return ret;
> 	}

Is coredump_setting_sem a global semaphore?  If so, it prevents concurrent
core dumping.  Additionally, while some process is dumped, writing to
coredump_omit_anon_shared of unrelated process will be blocked.

So we should use per process or per mm locking.  But where should we
place the variable for locking?  Because we don't want to increase the
struct size, we might want to add another bit field in mm_struct:

	struct mm_struct {
		...
 		unsigned char dumpable:2;
		unsigned char coredump_in_progress:1;      /* this */
		unsigned char coredump_omit_anon_shared:1;
		...

And we use it to determine whether core dumping is in progress or not:

 	int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	{
 		<setup vars>
 
 		spin_lock(&dump_bits_lock);
		current->mm->coredump_in_progress = 1;
		spin_unlock(&dump_bits_lock);
		...

Additionally:

 	static ssize_t proc_coredump_omit_anon_shared_write(struct file *file,
 						    const char __user *buf,
 						    size_t count,
 						    loff_t *ppos)
 	{
 		<setup vars>

		ret = -EBUSY; 
		spin_lock(&dump_bits_lock);
		if (mm->coredump_in_progress) {
			spin_unlock(&dump_bits_lock);
			goto out;
		}
		mm->coredump_omit_anon_shared = (val != 0);
		spin_unlock(&dump_bits_lock);
		...

Do you think which is better this method or passing argument method
used by my patchset?
Or, are there even better way else?

-- 
Hidehiro Kawai
Hitachi, Ltd., Systems Development Laboratory