[PATCH] [1/3] Some cleanup in the pipe code

[PATCH] [1/3] Some cleanup in the pipe code

[Andi Kleen]

Split the big and hard to read do_pipe function into smaller pieces.

This creates new create_write_pipe/free_write_pipe/create_read_pipe 
functions. These functions are made global so that they can be
used by other parts of the kernel.

The resulting code is more generic and easier to read and has cleaner error 
handling and less gotos.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -1563,6 +1563,9 @@ static inline void allow_write_access(st
 		atomic_inc(&file->f_dentry->d_inode->i_writecount);
 }
 extern int do_pipe(int *);
+extern struct file *create_read_pipe(struct file *f);
+extern struct file *create_write_pipe(void);
+extern void free_write_pipe(struct file *);
 
 extern int open_namei(int dfd, const char *, int, int, struct nameidata *);
 extern int may_open(struct nameidata *, int, int);
Index: linux/fs/pipe.c
===================================================================
--- linux.orig/fs/pipe.c
+++ linux/fs/pipe.c
@@ -890,87 +890,118 @@ fail_inode:
 	return NULL;
 }
 
-int do_pipe(int *fd)
+struct file *create_write_pipe(void)
 {
-	struct qstr this;
-	char name[32];
+	int err;
+	struct inode *inode;
+	struct file *f;
 	struct dentry *dentry;
-	struct inode * inode;
-	struct file *f1, *f2;
-	int error;
-	int i, j;
-
-	error = -ENFILE;
-	f1 = get_empty_filp();
-	if (!f1)
-		goto no_files;
-
-	f2 = get_empty_filp();
-	if (!f2)
-		goto close_f1;
+	char name[32];
+	struct qstr this;
 
+	f = get_empty_filp();
+	if (!f)
+		return ERR_PTR(-ENFILE);
+	err = -ENFILE;
 	inode = get_pipe_inode();
 	if (!inode)
-		goto close_f12;
-
-	error = get_unused_fd();
-	if (error < 0)
-		goto close_f12_inode;
-	i = error;
-
-	error = get_unused_fd();
-	if (error < 0)
-		goto close_f12_inode_i;
-	j = error;
+		goto err_file;
 
-	error = -ENOMEM;
 	sprintf(name, "[%lu]", inode->i_ino);
 	this.name = name;
 	this.len = strlen(name);
 	this.hash = inode->i_ino; /* will go */
+	err = -ENOMEM;
 	dentry = d_alloc(pipe_mnt->mnt_sb->s_root, &this);
 	if (!dentry)
-		goto close_f12_inode_i_j;
+		goto err_inode;
 
 	dentry->d_op = &pipefs_dentry_operations;
 	d_add(dentry, inode);
-	f1->f_vfsmnt = f2->f_vfsmnt = mntget(mntget(pipe_mnt));
-	f1->f_dentry = f2->f_dentry = dget(dentry);
-	f1->f_mapping = f2->f_mapping = inode->i_mapping;
-
-	/* read file */
-	f1->f_pos = f2->f_pos = 0;
-	f1->f_flags = O_RDONLY;
-	f1->f_op = &read_pipe_fops;
-	f1->f_mode = FMODE_READ;
-	f1->f_version = 0;
-
-	/* write file */
-	f2->f_flags = O_WRONLY;
-	f2->f_op = &write_pipe_fops;
-	f2->f_mode = FMODE_WRITE;
-	f2->f_version = 0;
+	f->f_vfsmnt = mntget(pipe_mnt);
+	f->f_dentry = dentry;
+	f->f_mapping = inode->i_mapping;
+
+	f->f_flags = O_WRONLY;
+	f->f_op = &write_pipe_fops;
+	f->f_mode = FMODE_WRITE;
+	f->f_version = 0;
+
+	return f;
+
+ err_inode:
+	free_pipe_info(inode);
+	iput(inode);
+ err_file:
+	put_filp(f);
+	return ERR_PTR(err);
+}
+
+void free_write_pipe(struct file *f)
+{
+	mntput(f->f_vfsmnt);
+	dput(f->f_dentry);
+	put_filp(f);
+}
+
+struct file *create_read_pipe(struct file *wrf)
+{
+	struct file *f = get_empty_filp();
+	if (!f)
+		return ERR_PTR(-ENFILE);
+
+	/* Grab pipe from the writer */
+	f->f_vfsmnt = mntget(wrf->f_vfsmnt);
+	f->f_dentry = dget(wrf->f_dentry);
+	f->f_mapping = wrf->f_dentry->d_inode->i_mapping;
+
+	f->f_pos = 0;
+	f->f_flags = O_RDONLY;
+	f->f_op = &read_pipe_fops;
+	f->f_mode = FMODE_READ;
+	f->f_version = 0;
+
+	return f;
+}
+
+int do_pipe(int *fd)
+{
+	struct file *fw, *fr;
+	int error;
+	int i, j;
 
-	fd_install(i, f1);
-	fd_install(j, f2);
+	fw = create_write_pipe();
+	if (IS_ERR(fw))
+		return PTR_ERR(fw);
+	fr = create_read_pipe(fw);
+	error = PTR_ERR(fr);
+	if (IS_ERR(fr))
+		goto err_write_pipe;
+
+	error = get_unused_fd();
+	if (error < 0)
+		goto err_read_pipe;
+	i = error;
+
+	error = get_unused_fd();
+	if (error < 0)
+		goto err_i_fd;
+	j = error;
+
+	fd_install(i, fr);
+	fd_install(j, fw);
 	fd[0] = i;
 	fd[1] = j;
 
 	return 0;
 
-close_f12_inode_i_j:
-	put_unused_fd(j);
-close_f12_inode_i:
+ err_i_fd:
 	put_unused_fd(i);
-close_f12_inode:
-	free_pipe_info(inode);
-	iput(inode);
-close_f12:
-	put_filp(f2);
-close_f1:
-	put_filp(f1);
-no_files:
-	return error;	
+ err_read_pipe:
+	put_filp(fr);
+ err_write_pipe:
+	free_write_pipe(fw);
+	return error;
 }
 
 /*

[PATCH] [2/3] Create call_usermodehelper_pipe()

[Andi Kleen]

A new member in the ever growing family of call_usermode* functions is born.
The new call_usermodehelper_pipe() function allows to pipe data to the stdin of the called
user mode progam and behaves otherwise like the normal call_usermodehelp()
(except that it always waits for the child to finish)

To be used in a follow up patch.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/kernel/kmod.c
===================================================================
--- linux.orig/kernel/kmod.c
+++ linux/kernel/kmod.c
@@ -122,6 +122,7 @@ struct subprocess_info {
 	struct key *ring;
 	int wait;
 	int retval;
+	struct file *stdin;
 };
 
 /*
@@ -145,12 +146,26 @@ static int ____call_usermodehelper(void 
 
 	key_put(old_session);
 
+	/* Install input pipe when needed */
+	if (sub_info->stdin) {
+		struct files_struct *f = current->files;
+		struct fdtable *fdt;
+		/* no races because files should be private here */
+		sys_close(0);
+		fd_install(0, sub_info->stdin);
+		spin_lock(&f->file_lock);
+		fdt = files_fdtable(f);
+		FD_SET(0, fdt->open_fds);
+		FD_CLR(0, fdt->close_on_exec);
+		spin_unlock(&f->file_lock);
+	}
+
 	/* We can run anywhere, unlike our parent keventd(). */
 	set_cpus_allowed(current, CPU_MASK_ALL);
 
 	retval = -EPERM;
 	if (current->fs->root)
-		retval = execve(sub_info->path, sub_info->argv,sub_info->envp);
+		retval = execve(sub_info->path, sub_info->argv, sub_info->envp);
 
 	/* Exec failed? */
 	sub_info->retval = retval;
@@ -257,6 +272,44 @@ int call_usermodehelper_keys(char *path,
 }
 EXPORT_SYMBOL(call_usermodehelper_keys);
 
+int call_usermodehelper_pipe(char *path, char **argv, char **envp,
+			     struct file **filp)
+{
+	DECLARE_COMPLETION(done);
+	struct subprocess_info sub_info = {
+		.complete	= &done,
+		.path		= path,
+		.argv		= argv,
+		.envp		= envp,
+		.retval		= 0,
+	};
+	struct file *f;
+	DECLARE_WORK(work, __call_usermodehelper, &sub_info);
+
+	if (!khelper_wq)
+		return -EBUSY;
+
+	if (path[0] == '\0')
+		return 0;
+
+	f = create_write_pipe();
+	if (!f)
+		return -ENOMEM;
+	*filp = f;
+
+	f = create_read_pipe(f);
+	if (!f) {
+		free_write_pipe(*filp);
+		return -ENOMEM;
+	}
+	sub_info.stdin = f;
+
+	queue_work(khelper_wq, &work);
+	wait_for_completion(&done);
+	return sub_info.retval;
+}
+EXPORT_SYMBOL(call_usermodehelper_pipe);
+
 void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
Index: linux/include/linux/kmod.h
===================================================================
--- linux.orig/include/linux/kmod.h
+++ linux/include/linux/kmod.h
@@ -47,4 +47,8 @@ call_usermodehelper(char *path, char **a
 
 extern void usermodehelper_init(void);
 
+struct file;
+extern int call_usermodehelper_pipe(char *path, char *argv[], char *envp[],
+				    struct file **filp);
+
 #endif /* __LINUX_KMOD_H__ */

Re: [PATCH] [2/3] Create call_usermodehelper_pipe()

[Andrew Morton]

On Mon, 14 Aug 2006 13:27:31 +0200 (CEST)
Andi Kleen <ak@suse.de> wrote:

> +	/* Install input pipe when needed */
> +	if (sub_info->stdin) {
> +		struct files_struct *f = current->files;
> +		struct fdtable *fdt;
> +		/* no races because files should be private here */
> +		sys_close(0);
> +		fd_install(0, sub_info->stdin);
> +		spin_lock(&f->file_lock);
> +		fdt = files_fdtable(f);
> +		FD_SET(0, fdt->open_fds);
> +		FD_CLR(0, fdt->close_on_exec);
> +		spin_unlock(&f->file_lock);
> +	}

This is all going to be run by kernel threads, and all kernel threads share
current->files=&init_files.

So I suspect that if two coredumps happen at the same time bad things will
happen.  Like a BUG() in fd_install()?

Re: [PATCH] [2/3] Create call_usermodehelper_pipe()

[Andrew Morton]

On Tue, 15 Aug 2006 14:22:25 -0700
Andrew Morton <akpm@osdl.org> wrote:

> On Mon, 14 Aug 2006 13:27:31 +0200 (CEST)
> Andi Kleen <ak@suse.de> wrote:
> 
> > +	/* Install input pipe when needed */
> > +	if (sub_info->stdin) {
> > +		struct files_struct *f = current->files;
> > +		struct fdtable *fdt;
> > +		/* no races because files should be private here */
> > +		sys_close(0);
> > +		fd_install(0, sub_info->stdin);
> > +		spin_lock(&f->file_lock);
> > +		fdt = files_fdtable(f);
> > +		FD_SET(0, fdt->open_fds);
> > +		FD_CLR(0, fdt->close_on_exec);
> > +		spin_unlock(&f->file_lock);
> > +	}
> 
> This is all going to be run by kernel threads, and all kernel threads share
> current->files=&init_files.

hm, that's wrong, isn't it - we're not using CLONE_FILES...

So what we have is a copy of init_files.  So the sys_close(0) shouldn't be needed?

Re: [PATCH] [2/3] Create call_usermodehelper_pipe()

[Andi Kleen]

 current->files=&init_files.
> 
> hm, that's wrong, isn't it - we're not using CLONE_FILES...

Yes, I thought I got that right but you shocked me quickly :)

> 
> So what we have is a copy of init_files.  So the sys_close(0) shouldn't be needed?

Probably not. It was just paranoia I guess.

-Andi

[PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andi Kleen]

Using the infrastructure created in previous patches implement support
to pipe core dumps into programs. 

This is done by overloading the existing core_pattern sysctl
with a new syntax:

|program

When the first character of the pattern is a '|' the kernel will
instead threat the rest of the pattern as a command to run. The 
core dump will be written to the standard input of that program
instead of to a file.

This is useful for having automatic core dump analysis without
filling up disks. The program can do some simple analysis and save only 
a summary of the core dump.

The core dump proces will run with the privileges and in the name space
of the process that caused the core dump.

I also increased the core pattern size to 128 bytes so that
longer command lines fit.

Most of the changes comes from allowing core dumps without seeks.
They are fairly straight forward though.

One small incompatibility is that if someone had a core pattern previously that
started with '|' they will get suddenly new behaviour. I think that's
unlikely to be a real problem though.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/fs/exec.c
===================================================================
--- linux.orig/fs/exec.c
+++ linux/fs/exec.c
@@ -58,7 +58,7 @@
 #endif
 
 int core_uses_pid;
-char core_pattern[65] = "core";
+char core_pattern[128] = "core";
 int suid_dumpable = 0;
 
 EXPORT_SYMBOL(suid_dumpable);
@@ -1475,6 +1475,7 @@ int do_coredump(long signr, int exit_cod
 	int retval = 0;
 	int fsuid = current->fsuid;
 	int flag = 0;
+	int ispipe = 0;
 
 	binfmt = current->binfmt;
 	if (!binfmt || !binfmt->core_dump)
@@ -1516,22 +1517,34 @@ int do_coredump(long signr, int exit_cod
  	lock_kernel();
 	format_corename(corename, core_pattern, signr);
 	unlock_kernel();
-	file = filp_open(corename, O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag, 0600);
+ 	if (corename[0] == '|') {
+		/* SIGPIPE can happen, but it's just never processed */
+ 		if(call_usermodehelper_pipe(corename+1, NULL, NULL, &file)) {
+ 			printk(KERN_INFO "Core dump to %s pipe failed\n",
+			       corename);
+ 			goto fail_unlock;
+ 		}
+		ispipe = 1;
+ 	} else
+ 		file = filp_open(corename,
+				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
 	if (IS_ERR(file))
 		goto fail_unlock;
 	inode = file->f_dentry->d_inode;
 	if (inode->i_nlink > 1)
 		goto close_fail;	/* multiple links - don't dump */
-	if (d_unhashed(file->f_dentry))
+	if (!ispipe && d_unhashed(file->f_dentry))
 		goto close_fail;
 
-	if (!S_ISREG(inode->i_mode))
+	/* AK: actually i see no reason to not allow this for named pipes etc.,
+	   but keep the previous behaviour for now. */
+	if (!ispipe && !S_ISREG(inode->i_mode))
 		goto close_fail;
 	if (!file->f_op)
 		goto close_fail;
 	if (!file->f_op->write)
 		goto close_fail;
-	if (do_truncate(file->f_dentry, 0, 0, file) != 0)
+	if (!ispipe && do_truncate(file->f_dentry, 0, 0, file) != 0)
 		goto close_fail;
 
 	retval = binfmt->core_dump(signr, regs, file);
Index: linux/fs/binfmt_elf.c
===================================================================
--- linux.orig/fs/binfmt_elf.c
+++ linux/fs/binfmt_elf.c
@@ -1153,11 +1153,23 @@ static int dump_write(struct file *file,
 
 static int dump_seek(struct file *file, loff_t off)
 {
-	if (file->f_op->llseek) {
-		if (file->f_op->llseek(file, off, 0) != off)
+	if (file->f_op->llseek && file->f_op->llseek != no_llseek) {
+		if (file->f_op->llseek(file, off, 1) != off)
 			return 0;
-	} else
-		file->f_pos = off;
+	} else {
+		char *buf = (char *)get_zeroed_page(GFP_KERNEL);
+		if (!buf)
+			return 0;
+		while (off > 0) {
+			unsigned long n = off;
+			if (n > PAGE_SIZE)
+				n = PAGE_SIZE;
+			if (!dump_write(file, buf, n))
+				return 0;
+			off -= n;
+		}
+		free_page((unsigned long)buf);
+	}
 	return 1;
 }
 
@@ -1205,30 +1217,32 @@ static int notesize(struct memelfnote *e
 	return sz;
 }
 
-#define DUMP_WRITE(addr, nr)	\
-	do { if (!dump_write(file, (addr), (nr))) return 0; } while(0)
-#define DUMP_SEEK(off)	\
-	do { if (!dump_seek(file, (off))) return 0; } while(0)
+#define DUMP_WRITE(addr, nr, foffset)	\
+	do { if (!dump_write(file, (addr), (nr))) return 0; *foffset += (nr); } while(0)
 
-static int writenote(struct memelfnote *men, struct file *file)
+static int alignfile(struct file *file, unsigned long *foffset)
 {
-	struct elf_note en;
+	char buf[4] = { 0, };
+	DUMP_WRITE(buf, roundup(*foffset, 4) - *foffset, foffset);
+	return 1;
+}
 
+static int writenote(struct memelfnote *men, struct file *file, unsigned long *foffset)
+{
+	struct elf_note en;
 	en.n_namesz = strlen(men->name) + 1;
 	en.n_descsz = men->datasz;
 	en.n_type = men->type;
 
-	DUMP_WRITE(&en, sizeof(en));
-	DUMP_WRITE(men->name, en.n_namesz);
-	/* XXX - cast from long long to long to avoid need for libgcc.a */
-	DUMP_SEEK(roundup((unsigned long)file->f_pos, 4));	/* XXX */
-	DUMP_WRITE(men->data, men->datasz);
-	DUMP_SEEK(roundup((unsigned long)file->f_pos, 4));	/* XXX */
+	DUMP_WRITE(&en, sizeof(en), foffset);
+	DUMP_WRITE(men->name, en.n_namesz, foffset);
+	if (!alignfile(file, foffset)) return 0;
+	DUMP_WRITE(men->data, men->datasz, foffset);
+	if (!alignfile(file, foffset)) return 0;
 
 	return 1;
 }
 #undef DUMP_WRITE
-#undef DUMP_SEEK
 
 #define DUMP_WRITE(addr, nr)	\
 	if ((size += (nr)) > limit || !dump_write(file, (addr), (nr))) \
@@ -1428,7 +1442,7 @@ static int elf_core_dump(long signr, str
 	int i;
 	struct vm_area_struct *vma;
 	struct elfhdr *elf = NULL;
-	off_t offset = 0, dataoff;
+	off_t offset = 0, dataoff, foffset;
 	unsigned long limit = current->signal->rlim[RLIMIT_CORE].rlim_cur;
 	int numnote;
 	struct memelfnote *notes = NULL;
@@ -1572,7 +1586,8 @@ static int elf_core_dump(long signr, str
 		DUMP_WRITE(&phdr, sizeof(phdr));
 	}
 
-	/* Page-align dumped data */
+	foffset = offset;
+
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
 	/* Write program headers for segments dump */
@@ -1597,6 +1612,7 @@ static int elf_core_dump(long signr, str
 		phdr.p_align = ELF_EXEC_PAGESIZE;
 
 		DUMP_WRITE(&phdr, sizeof(phdr));
+		foffset += sizeof(phdr);
 	}
 
 #ifdef ELF_CORE_WRITE_EXTRA_PHDRS
@@ -1605,7 +1621,7 @@ static int elf_core_dump(long signr, str
 
  	/* write out the notes section */
 	for (i = 0; i < numnote; i++)
-		if (!writenote(notes + i, file))
+		if (!writenote(notes + i, file, &foffset))
 			goto end_coredump;
 
 	/* write out the thread status notes section */
@@ -1614,11 +1630,12 @@ static int elf_core_dump(long signr, str
 				list_entry(t, struct elf_thread_status, list);
 
 		for (i = 0; i < tmp->num_notes; i++)
-			if (!writenote(&tmp->notes[i], file))
+			if (!writenote(&tmp->notes[i], file, &foffset))
 				goto end_coredump;
 	}
- 
-	DUMP_SEEK(dataoff);
+
+	/* Align to page */
+	DUMP_SEEK(dataoff - foffset);
 
 	for (vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
 		unsigned long addr;
@@ -1634,10 +1651,10 @@ static int elf_core_dump(long signr, str
 
 			if (get_user_pages(current, current->mm, addr, 1, 0, 1,
 						&page, &vma) <= 0) {
-				DUMP_SEEK(file->f_pos + PAGE_SIZE);
+				DUMP_SEEK(PAGE_SIZE);
 			} else {
 				if (page == ZERO_PAGE(addr)) {
-					DUMP_SEEK(file->f_pos + PAGE_SIZE);
+					DUMP_SEEK(PAGE_SIZE);
 				} else {
 					void *kaddr;
 					flush_cache_page(vma, addr,
@@ -1661,13 +1678,6 @@ static int elf_core_dump(long signr, str
 	ELF_CORE_WRITE_EXTRA_DATA;
 #endif
 
-	if ((off_t)file->f_pos != offset) {
-		/* Sanity check */
-		printk(KERN_WARNING
-		       "elf_core_dump: file->f_pos (%ld) != offset (%ld)\n",
-		       (off_t)file->f_pos, offset);
-	}
-
 end_coredump:
 	set_fs(fs);
 
Index: linux/kernel/sysctl.c
===================================================================
--- linux.orig/kernel/sysctl.c
+++ linux/kernel/sysctl.c
@@ -293,7 +293,7 @@ static ctl_table kern_table[] = {
 		.ctl_name	= KERN_CORE_PATTERN,
 		.procname	= "core_pattern",
 		.data		= core_pattern,
-		.maxlen		= 64,
+		.maxlen		= 128,
 		.mode		= 0644,
 		.proc_handler	= &proc_dostring,
 		.strategy	= &sysctl_string,

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andrew Morton]

On Mon, 14 Aug 2006 13:27:32 +0200 (CEST)
Andi Kleen <ak@suse.de> wrote:

> The core dump proces will run with the privileges and in the name space
> of the process that caused the core dump.

Don't think so.   __call_usermodehelper() is executed by the khelper kernel thread.

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andi Kleen]

On Mon, Aug 14, 2006 at 05:43:19PM -0700, Andrew Morton wrote:
> On Mon, 14 Aug 2006 13:27:32 +0200 (CEST)
> Andi Kleen <ak@suse.de> wrote:
> 
> > The core dump proces will run with the privileges and in the name space
> > of the process that caused the core dump.
> 
> Don't think so.   __call_usermodehelper() is executed by the khelper kernel thread.

Yes, you're right the comment is wrong. It does run as root instead
with global name space. I needed that because otherwise
it needed a global writable directory for the core dumps. 

Feel free to change it in your version.

That happens when you write the description months later than the code ...

-Andi

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Greg KH]

On Mon, Aug 14, 2006 at 01:27:32PM +0200, Andi Kleen wrote:
> 
> Using the infrastructure created in previous patches implement support
> to pipe core dumps into programs. 
> 
> This is done by overloading the existing core_pattern sysctl
> with a new syntax:
> 
> |program

Very nice, do you happen to have a program that can accept this kind of
input for crash dumps?  I'm guessing that the embedded people will
really want this functionality.

thanks,

greg k-h

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andi Kleen]

On Wed, 16 Aug 2006 01:43:54 -0700
Greg KH <greg@kroah.com> wrote:

> On Mon, Aug 14, 2006 at 01:27:32PM +0200, Andi Kleen wrote:
> > 
> > Using the infrastructure created in previous patches implement support
> > to pipe core dumps into programs. 
> > 
> > This is done by overloading the existing core_pattern sysctl
> > with a new syntax:
> > 
> > |program
> 
> Very nice, do you happen to have a program that can accept this kind of
> input for crash dumps?  I'm guessing that the embedded people will
> really want this functionality.

I had a cheesy demo/prototype. Basically it wrote the dump to a file again,
ran gdb on it to get a backtrace and wrote the summary to a shared directory.
Then there was a simple CGI script to generate a "top 10" crashes
HTML listing.

Unfortunately this still had the disadvantage to needing full disk space
for a dump except for deleting it afterwards (in fact it was worse because 
over the pipe holes didn't work so if you have a holey address map it would 
require more space).

Fortunately gdb seems to be happy to handle /proc/pid/fd/xxx input pipes
as cores (at least it worked with zsh's =(cat core) syntax), so it would be 
likely possible to do it without temporary space with a simple wrapper that 
calls it in the right way.  I ran out of time before doing that though.

The demo prototype scripts weren't very good. If there is really interest
I can dig them out (they are currently on a laptop disk on the desk 
with the laptop itself being in service), but I would recommend to 
rewrite them for any  serious application of this and fix the disk space problem.

Also to be really useful it should probably find a way to automatically
fetch the debuginfos (I cheated and just installed them in advance)

If nobody else does it I can probably do the rewrite myself again at some point.

My hope at some point was that desktops would support it in their
builtin crash reporters, but at least the KDE people I talked
too seemed to be happy with their user space only solution.

-Andi

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andrew Morton]

On Wed, 16 Aug 2006 11:18:01 +0200
Andi Kleen <ak@muc.de> wrote:

> > Very nice, do you happen to have a program that can accept this kind of
> > input for crash dumps?  I'm guessing that the embedded people will
> > really want this functionality.
> 
> I had a cheesy demo/prototype. Basically it wrote the dump to a file again,
> ran gdb on it to get a backtrace and wrote the summary to a shared directory.
> Then there was a simple CGI script to generate a "top 10" crashes
> HTML listing.
> 
> Unfortunately this still had the disadvantage to needing full disk space
> for a dump except for deleting it afterwards (in fact it was worse because 
> over the pipe holes didn't work so if you have a holey address map it would 
> require more space).
> 
> Fortunately gdb seems to be happy to handle /proc/pid/fd/xxx input pipes
> as cores (at least it worked with zsh's =(cat core) syntax), so it would be 
> likely possible to do it without temporary space with a simple wrapper that 
> calls it in the right way.  I ran out of time before doing that though.
> 
> The demo prototype scripts weren't very good. If there is really interest
> I can dig them out (they are currently on a laptop disk on the desk 
> with the laptop itself being in service), but I would recommend to 
> rewrite them for any  serious application of this and fix the disk space problem.
> 
> Also to be really useful it should probably find a way to automatically
> fetch the debuginfos (I cheated and just installed them in advance)
> 
> If nobody else does it I can probably do the rewrite myself again at some point.
> 
> My hope at some point was that desktops would support it in their
> builtin crash reporters, but at least the KDE people I talked
> too seemed to be happy with their user space only solution.

It doens't sounds like there's particularly strong userspace "pull" for
this feature?

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andi Kleen]

> It doens't sounds like there's particularly strong userspace "pull" for
> this feature?

Several people from the embedded area wrote me privately
it would be useful for them. Also I think once it's in the main kernel
there will be more incentive for user space to use it and I'm optimistic
it will get some adoption (ok I guess I should write some better
documentation, but there was no obvious place for it)

-Andi

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Alan Cox]

Ar Iau, 2006-08-17 am 11:46 +0200, ysgrifennodd Andi Kleen:
> Several people from the embedded area wrote me privately
> it would be useful for them. Also I think once it's in the main kernel
> there will be more incentive for user space to use it and I'm optimistic
> it will get some adoption (ok I guess I should write some better
> documentation, but there was no obvious place for it)

I don't believe that piping as such as neccessarily the right model, but
the ability to intercept and processes core dumps from user space is
asked for by many enterprise users as well. They want to know about,
capture, analyse and process core dumps, often centrally and in
automated form.

Alan

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andrew Morton]

On Thu, 17 Aug 2006 12:27:44 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> Ar Iau, 2006-08-17 am 11:46 +0200, ysgrifennodd Andi Kleen:
> > Several people from the embedded area wrote me privately
> > it would be useful for them. Also I think once it's in the main kernel
> > there will be more incentive for user space to use it and I'm optimistic
> > it will get some adoption (ok I guess I should write some better
> > documentation, but there was no obvious place for it)
> 
> I don't believe that piping as such as neccessarily the right model, but
> the ability to intercept and processes core dumps from user space is
> asked for by many enterprise users as well. They want to know about,
> capture, analyse and process core dumps, often centrally and in
> automated form.
> 

OK, fair enough.

Now let's think about security.  Patches against ptrace, coredump and
procfs give me the creeps because we've had (relatively) so many problems
in those areas in the past.

So I'd suggest that we should look at this code and think about it in a
really twisted fashion - does it open any exploits?  I can't think of any,
which is worth practically zero, but I don't see how this differs from
/proc/sys/kernel/modprobe.

But still.   Is this code secure?

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Andi Kleen]

> But still.   Is this code secure?

Any auditing from third parties appreciated.

I don't know of any obvious flaws (at least assuming the pipe handler
isn't insecure code), but then I'm biased. 

-Andi

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Arjan van de Ven]

On Fri, 2006-08-18 at 10:29 +0200, Andi Kleen wrote:
> > But still.   Is this code secure?
> 
> Any auditing from third parties appreciated.
> 
> I don't know of any obvious flaws (at least assuming the pipe handler
> isn't insecure code), but then I'm biased. 

one angle is... is the userspace handler trusted code by root? Or is it
allowed to be per user/user defined binary? In the later case there are
all kinds of DoS scenarios possible; in the former it's root doing that
to himself ;)

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [PATCH] [3/3] Support piping into commands in /proc/sys/kernel/core_pattern

[Arjan van de Ven]

On Fri, 2006-08-18 at 14:01 +0200, Arjan van de Ven wrote:
> On Fri, 2006-08-18 at 10:29 +0200, Andi Kleen wrote:
> > > But still.   Is this code secure?
> > 
> > Any auditing from third parties appreciated.
> > 
> > I don't know of any obvious flaws (at least assuming the pipe handler
> > isn't insecure code), but then I'm biased. 
> 
> one angle is... is the userspace handler trusted code by root? Or is it
> allowed to be per user/user defined binary? In the later case there are
> all kinds of DoS scenarios possible; in the former it's root doing that
> to himself ;)

so one angle that needs to be resolved is the selinux interaction, where
root!=root to some degree. If "any root" can control the sysctl, then
this could lead to information leaks, or worse, to a "limited root" to
"full root" escalation...

> 
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com