Re: Yet another pipe related oops.
On Tue, Apr 02, 2013 at 01:22:04AM +0100, Al Viro wrote:
> On Tue, Apr 02, 2013 at 12:27:18AM +0100, Al Viro wrote:
> > On Mon, Apr 01, 2013 at 02:44:36PM -0700, Greg Kroah-Hartman wrote:
> > > > > I guess you are right, it will not. I guess we need to do what
> > > > > character devices do and have an "intermediate" fops in order to
> > > > > protect
> > > > > this. Would that work?
> > > >
> > > > You mean, with reassigning ->f_op in ->open()? That'll work, as long as
> > > > we have exclusion between removal and fetching the sucker in primary
> > > > ->open()... Where would you prefer to stash fops?
> > >
> > > Ick, that's not going to work as the current api just uses a fops and
> > > debugfs doesn't keep anything else hanging around that referes to
> > > something "before" that, like 'struct cdev' does.
> >
> > Er? How about just sticking it into dentry->d_fsdata and letting
> > debugfs_remove() zero that out? What am I missing here?
Nothing, you are right, that would work just fine. Want me to fix it
up, or do you want to?
> Hrm... For what it's worth, how do debugfs entries associated with
> dynamic objects deal with debugfs_remove() vs. method calls? I don't
> see _anything_ in {,__}debugfs_remove() that would looks like "wait
> for ongoing write(2) attempts to complete". IOW, forget rmmod - WTF
> protects us from access-after-free for any kind of data that isn't
> permanently allocated?
Nothing protects you from that, that's what I was trying to get at with
the dynamic attributes comment.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Tue, Apr 02, 2013 at 12:27:18AM +0100, Al Viro wrote:
> On Mon, Apr 01, 2013 at 02:44:36PM -0700, Greg Kroah-Hartman wrote:
> > > > I guess you are right, it will not. I guess we need to do what
> > > > character devices do and have an "intermediate" fops in order to protect
> > > > this. Would that work?
> > >
> > > You mean, with reassigning ->f_op in ->open()? That'll work, as long as
> > > we have exclusion between removal and fetching the sucker in primary
> > > ->open()... Where would you prefer to stash fops?
> >
> > Ick, that's not going to work as the current api just uses a fops and
> > debugfs doesn't keep anything else hanging around that referes to
> > something "before" that, like 'struct cdev' does.
>
> Er? How about just sticking it into dentry->d_fsdata and letting
> debugfs_remove() zero that out? What am I missing here?
Hrm... For what it's worth, how do debugfs entries associated with
dynamic objects deal with debugfs_remove() vs. method calls? I don't
see _anything_ in {,__}debugfs_remove() that would looks like "wait
for ongoing write(2) attempts to complete". IOW, forget rmmod - WTF
protects us from access-after-free for any kind of data that isn't
permanently allocated?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 02:44:36PM -0700, Greg Kroah-Hartman wrote: > > > I guess you are right, it will not. I guess we need to do what > > > character devices do and have an "intermediate" fops in order to protect > > > this. Would that work? > > > > You mean, with reassigning ->f_op in ->open()? That'll work, as long as > > we have exclusion between removal and fetching the sucker in primary > > ->open()... Where would you prefer to stash fops? > > Ick, that's not going to work as the current api just uses a fops and > debugfs doesn't keep anything else hanging around that referes to > something "before" that, like 'struct cdev' does. Er? How about just sticking it into dentry->d_fsdata and letting debugfs_remove() zero that out? What am I missing here? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 10:21:42PM +0100, Al Viro wrote: > On Mon, Apr 01, 2013 at 02:00:29PM -0700, Greg Kroah-Hartman wrote: > > > > IOW, how do we deal with a race between attempt to open a debugfs file and > > > its removal on driver unload? Greg? > > > > Hm, I thought the i_fop->owner thing would be the needed protection, but > > It will be, if you manage to fetch it... I agree. > > I guess you are right, it will not. I guess we need to do what > > character devices do and have an "intermediate" fops in order to protect > > this. Would that work? > > You mean, with reassigning ->f_op in ->open()? That'll work, as long as > we have exclusion between removal and fetching the sucker in primary > ->open()... Where would you prefer to stash fops? Ick, that's not going to work as the current api just uses a fops and debugfs doesn't keep anything else hanging around that referes to something "before" that, like 'struct cdev' does. And, it's even worse, look at the use of DEFINE_SIMPLE_ATTRIBUTE(), those take a pointer from a random module to read/write from, and use the fops for the debugfs module. Hopefully no other user of that macro has the same problem, and at first glance, I think that's true, but I might be wrong... Am I allowed to "punt" and say, "removing a module that uses debugfs is not recommended?" :) greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 02:00:29PM -0700, Greg Kroah-Hartman wrote: > > IOW, how do we deal with a race between attempt to open a debugfs file and > > its removal on driver unload? Greg? > > Hm, I thought the i_fop->owner thing would be the needed protection, but It will be, if you manage to fetch it... > I guess you are right, it will not. I guess we need to do what > character devices do and have an "intermediate" fops in order to protect > this. Would that work? You mean, with reassigning ->f_op in ->open()? That'll work, as long as we have exclusion between removal and fetching the sucker in primary ->open()... Where would you prefer to stash fops? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 09:34:46PM +0100, Al Viro wrote: > On Wed, Mar 27, 2013 at 05:45:06PM +, Al Viro wrote: > > We shouldn't, at least not for something that has been successfully > > opened. I've a patch series cleaning that up a bit in the local > > queue; will check for bitrot and throw into for-next. > > Egads... OK, that has gone more than slightly out of control - right now > vfs.git#for-next is at 106 commits, -3.6KLoC balance and *still* hadn't > reached the ->f_op part of that work ;-/ OTOH, procfs-related code got > a lot cleaner and we actually have a chance to make the guts of proc_dir_entry > private to fs/proc now... I'll cull the outright bug fixes into for-linus > and push it your way... > > The thing that really worries me is debugfs; that fscker sets whatever > file_operations it's got from the driver that registered a file there > and sticks that into ->i_fop. When we try to open() that, we get > try_module_get() ->i_fop->owner; so far, so good, but what if the driver > has just been removed *and* file_operations instance we are looking at > has already been freed? > > IOW, how do we deal with a race between attempt to open a debugfs file and > its removal on driver unload? Greg? Hm, I thought the i_fop->owner thing would be the needed protection, but I guess you are right, it will not. I guess we need to do what character devices do and have an "intermediate" fops in order to protect this. Would that work? As module removal never happens unless an admin does it by hand, it's not a "real" issue that can be triggered by anyone easily, thankfully. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 05:45:06PM +, Al Viro wrote: > We shouldn't, at least not for something that has been successfully > opened. I've a patch series cleaning that up a bit in the local > queue; will check for bitrot and throw into for-next. Egads... OK, that has gone more than slightly out of control - right now vfs.git#for-next is at 106 commits, -3.6KLoC balance and *still* hadn't reached the ->f_op part of that work ;-/ OTOH, procfs-related code got a lot cleaner and we actually have a chance to make the guts of proc_dir_entry private to fs/proc now... I'll cull the outright bug fixes into for-linus and push it your way... The thing that really worries me is debugfs; that fscker sets whatever file_operations it's got from the driver that registered a file there and sticks that into ->i_fop. When we try to open() that, we get try_module_get() ->i_fop->owner; so far, so good, but what if the driver has just been removed *and* file_operations instance we are looking at has already been freed? IOW, how do we deal with a race between attempt to open a debugfs file and its removal on driver unload? Greg? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 05:45:06PM +, Al Viro wrote: We shouldn't, at least not for something that has been successfully opened. I've a patch series cleaning that up a bit in the local queue; will check for bitrot and throw into for-next. Egads... OK, that has gone more than slightly out of control - right now vfs.git#for-next is at 106 commits, -3.6KLoC balance and *still* hadn't reached the -f_op part of that work ;-/ OTOH, procfs-related code got a lot cleaner and we actually have a chance to make the guts of proc_dir_entry private to fs/proc now... I'll cull the outright bug fixes into for-linus and push it your way... The thing that really worries me is debugfs; that fscker sets whatever file_operations it's got from the driver that registered a file there and sticks that into -i_fop. When we try to open() that, we get try_module_get() -i_fop-owner; so far, so good, but what if the driver has just been removed *and* file_operations instance we are looking at has already been freed? IOW, how do we deal with a race between attempt to open a debugfs file and its removal on driver unload? Greg? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 09:34:46PM +0100, Al Viro wrote: On Wed, Mar 27, 2013 at 05:45:06PM +, Al Viro wrote: We shouldn't, at least not for something that has been successfully opened. I've a patch series cleaning that up a bit in the local queue; will check for bitrot and throw into for-next. Egads... OK, that has gone more than slightly out of control - right now vfs.git#for-next is at 106 commits, -3.6KLoC balance and *still* hadn't reached the -f_op part of that work ;-/ OTOH, procfs-related code got a lot cleaner and we actually have a chance to make the guts of proc_dir_entry private to fs/proc now... I'll cull the outright bug fixes into for-linus and push it your way... The thing that really worries me is debugfs; that fscker sets whatever file_operations it's got from the driver that registered a file there and sticks that into -i_fop. When we try to open() that, we get try_module_get() -i_fop-owner; so far, so good, but what if the driver has just been removed *and* file_operations instance we are looking at has already been freed? IOW, how do we deal with a race between attempt to open a debugfs file and its removal on driver unload? Greg? Hm, I thought the i_fop-owner thing would be the needed protection, but I guess you are right, it will not. I guess we need to do what character devices do and have an intermediate fops in order to protect this. Would that work? As module removal never happens unless an admin does it by hand, it's not a real issue that can be triggered by anyone easily, thankfully. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 02:00:29PM -0700, Greg Kroah-Hartman wrote: IOW, how do we deal with a race between attempt to open a debugfs file and its removal on driver unload? Greg? Hm, I thought the i_fop-owner thing would be the needed protection, but It will be, if you manage to fetch it... I guess you are right, it will not. I guess we need to do what character devices do and have an intermediate fops in order to protect this. Would that work? You mean, with reassigning -f_op in -open()? That'll work, as long as we have exclusion between removal and fetching the sucker in primary -open()... Where would you prefer to stash fops? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 10:21:42PM +0100, Al Viro wrote: On Mon, Apr 01, 2013 at 02:00:29PM -0700, Greg Kroah-Hartman wrote: IOW, how do we deal with a race between attempt to open a debugfs file and its removal on driver unload? Greg? Hm, I thought the i_fop-owner thing would be the needed protection, but It will be, if you manage to fetch it... I agree. I guess you are right, it will not. I guess we need to do what character devices do and have an intermediate fops in order to protect this. Would that work? You mean, with reassigning -f_op in -open()? That'll work, as long as we have exclusion between removal and fetching the sucker in primary -open()... Where would you prefer to stash fops? Ick, that's not going to work as the current api just uses a fops and debugfs doesn't keep anything else hanging around that referes to something before that, like 'struct cdev' does. And, it's even worse, look at the use of DEFINE_SIMPLE_ATTRIBUTE(), those take a pointer from a random module to read/write from, and use the fops for the debugfs module. Hopefully no other user of that macro has the same problem, and at first glance, I think that's true, but I might be wrong... Am I allowed to punt and say, removing a module that uses debugfs is not recommended? :) greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Mon, Apr 01, 2013 at 02:44:36PM -0700, Greg Kroah-Hartman wrote: I guess you are right, it will not. I guess we need to do what character devices do and have an intermediate fops in order to protect this. Would that work? You mean, with reassigning -f_op in -open()? That'll work, as long as we have exclusion between removal and fetching the sucker in primary -open()... Where would you prefer to stash fops? Ick, that's not going to work as the current api just uses a fops and debugfs doesn't keep anything else hanging around that referes to something before that, like 'struct cdev' does. Er? How about just sticking it into dentry-d_fsdata and letting debugfs_remove() zero that out? What am I missing here? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Tue, Apr 02, 2013 at 12:27:18AM +0100, Al Viro wrote:
On Mon, Apr 01, 2013 at 02:44:36PM -0700, Greg Kroah-Hartman wrote:
I guess you are right, it will not. I guess we need to do what
character devices do and have an intermediate fops in order to protect
this. Would that work?
You mean, with reassigning -f_op in -open()? That'll work, as long as
we have exclusion between removal and fetching the sucker in primary
-open()... Where would you prefer to stash fops?
Ick, that's not going to work as the current api just uses a fops and
debugfs doesn't keep anything else hanging around that referes to
something before that, like 'struct cdev' does.
Er? How about just sticking it into dentry-d_fsdata and letting
debugfs_remove() zero that out? What am I missing here?
Hrm... For what it's worth, how do debugfs entries associated with
dynamic objects deal with debugfs_remove() vs. method calls? I don't
see _anything_ in {,__}debugfs_remove() that would looks like wait
for ongoing write(2) attempts to complete. IOW, forget rmmod - WTF
protects us from access-after-free for any kind of data that isn't
permanently allocated?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Tue, Apr 02, 2013 at 01:22:04AM +0100, Al Viro wrote:
On Tue, Apr 02, 2013 at 12:27:18AM +0100, Al Viro wrote:
On Mon, Apr 01, 2013 at 02:44:36PM -0700, Greg Kroah-Hartman wrote:
I guess you are right, it will not. I guess we need to do what
character devices do and have an intermediate fops in order to
protect
this. Would that work?
You mean, with reassigning -f_op in -open()? That'll work, as long as
we have exclusion between removal and fetching the sucker in primary
-open()... Where would you prefer to stash fops?
Ick, that's not going to work as the current api just uses a fops and
debugfs doesn't keep anything else hanging around that referes to
something before that, like 'struct cdev' does.
Er? How about just sticking it into dentry-d_fsdata and letting
debugfs_remove() zero that out? What am I missing here?
Nothing, you are right, that would work just fine. Want me to fix it
up, or do you want to?
Hrm... For what it's worth, how do debugfs entries associated with
dynamic objects deal with debugfs_remove() vs. method calls? I don't
see _anything_ in {,__}debugfs_remove() that would looks like wait
for ongoing write(2) attempts to complete. IOW, forget rmmod - WTF
protects us from access-after-free for any kind of data that isn't
permanently allocated?
Nothing protects you from that, that's what I was trying to get at with
the dynamic attributes comment.
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 09:33:35AM -0700, Linus Torvalds wrote: > Applied. > > Do we actually have files with NULL f_ops pointers? Should we? What > could we possibly do with a file descriptor that doesn't have any > fops? We shouldn't, at least not for something that has been successfully opened. I've a patch series cleaning that up a bit in the local queue; will check for bitrot and throw into for-next. Another thing that is a definite for-next fodder - we really have no reason to put anything non-regular or opened not for write into ->s_files. And since read-only opens outnumber write-only/read-write ones by far (two orders of magnitude for something like kernel build), that gives a nice reduction of files_lglock accesses. OTOH, the only remaining user of those lists is forced remount to read-only, and I'm not at all sure we wouldn't be better off by leaving those opened files alone and just teaching file_start_write() to fail with EROFS on such fs. Then we could get rid of files_lglock and ->s_files completely... > Also, perhaps we should do something more akin to what we do for > dentry functions where we validate them on registration, and we could > fix up or validate read/write pointers, with semantics something like > > if (!fop->write) > fop->write = fop->aio_write ? do_sync_write : EINVAL_write; > if (!fop->read) > fop->read = fop->aio_read ? do_sync_read : EINVAL_read; > > kind of things? As it is, file_operations instances are const, and it's a good idea, IMO... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 9:33 AM, Linus Torvalds wrote: > On Wed, Mar 27, 2013 at 8:20 AM, Al Viro wrote: >> >> Actually, that's my fault - check lost in patch reordering. My apologies ;-/ >> Eventually, we want that in fs/splice.c side of things (no point repeating it >> for every buffer, after all), but for now this is the obvious minimal fix. > > Applied. > > Do we actually have files with NULL f_ops pointers? Should we? What > could we possibly do with a file descriptor that doesn't have any > fops? For the sake of the curious including myself: How would such a NULL f_ops file get created in the first place? > Also, perhaps we should do something more akin to what we do for > dentry functions where we validate them on registration, and we could > fix up or validate read/write pointers, with semantics something like > > if (!fop->write) > fop->write = fop->aio_write ? do_sync_write : EINVAL_write; > if (!fop->read) > fop->read = fop->aio_read ? do_sync_read : EINVAL_read; > > kind of things? > > Not a big deal, perhaps. > > Linus > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 8:20 AM, Al Viro wrote: > > Actually, that's my fault - check lost in patch reordering. My apologies ;-/ > Eventually, we want that in fs/splice.c side of things (no point repeating it > for every buffer, after all), but for now this is the obvious minimal fix. Applied. Do we actually have files with NULL f_ops pointers? Should we? What could we possibly do with a file descriptor that doesn't have any fops? Also, perhaps we should do something more akin to what we do for dentry functions where we validate them on registration, and we could fix up or validate read/write pointers, with semantics something like if (!fop->write) fop->write = fop->aio_write ? do_sync_write : EINVAL_write; if (!fop->read) fop->read = fop->aio_read ? do_sync_read : EINVAL_read; kind of things? Not a big deal, perhaps. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 09:51:27AM -0400, Dave Jones wrote: > Could be that Al's patches refactored this bug away, or it could just be > that I've been lucky the last few weeks, and just haven't had the right > entropy to get the sequence of events right.. > > thoughts ? Actually, that's my fault - check lost in patch reordering. My apologies ;-/ Eventually, we want that in fs/splice.c side of things (no point repeating it for every buffer, after all), but for now this is the obvious minimal fix. Signed-off-by: Al Viro --- diff --git a/fs/read_write.c b/fs/read_write.c index f7b5a23..e6ddc8d 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -424,6 +424,9 @@ ssize_t __kernel_write(struct file *file, const char *buf, size_t count, loff_t const char __user *p; ssize_t ret; + if (!file->f_op || (!file->f_op->write && !file->f_op->aio_write)) + return -EINVAL; + old_fs = get_fs(); set_fs(get_ds()); p = (__force const char __user *)buf; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 09:51:27AM -0400, Dave Jones wrote: Could be that Al's patches refactored this bug away, or it could just be that I've been lucky the last few weeks, and just haven't had the right entropy to get the sequence of events right.. thoughts ? Actually, that's my fault - check lost in patch reordering. My apologies ;-/ Eventually, we want that in fs/splice.c side of things (no point repeating it for every buffer, after all), but for now this is the obvious minimal fix. Signed-off-by: Al Viro v...@zeniv.linux.org.uk --- diff --git a/fs/read_write.c b/fs/read_write.c index f7b5a23..e6ddc8d 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -424,6 +424,9 @@ ssize_t __kernel_write(struct file *file, const char *buf, size_t count, loff_t const char __user *p; ssize_t ret; + if (!file-f_op || (!file-f_op-write !file-f_op-aio_write)) + return -EINVAL; + old_fs = get_fs(); set_fs(get_ds()); p = (__force const char __user *)buf; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 8:20 AM, Al Viro v...@zeniv.linux.org.uk wrote: Actually, that's my fault - check lost in patch reordering. My apologies ;-/ Eventually, we want that in fs/splice.c side of things (no point repeating it for every buffer, after all), but for now this is the obvious minimal fix. Applied. Do we actually have files with NULL f_ops pointers? Should we? What could we possibly do with a file descriptor that doesn't have any fops? Also, perhaps we should do something more akin to what we do for dentry functions where we validate them on registration, and we could fix up or validate read/write pointers, with semantics something like if (!fop-write) fop-write = fop-aio_write ? do_sync_write : EINVAL_write; if (!fop-read) fop-read = fop-aio_read ? do_sync_read : EINVAL_read; kind of things? Not a big deal, perhaps. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 9:33 AM, Linus Torvalds torva...@linux-foundation.org wrote: On Wed, Mar 27, 2013 at 8:20 AM, Al Viro v...@zeniv.linux.org.uk wrote: Actually, that's my fault - check lost in patch reordering. My apologies ;-/ Eventually, we want that in fs/splice.c side of things (no point repeating it for every buffer, after all), but for now this is the obvious minimal fix. Applied. Do we actually have files with NULL f_ops pointers? Should we? What could we possibly do with a file descriptor that doesn't have any fops? For the sake of the curious including myself: How would such a NULL f_ops file get created in the first place? Also, perhaps we should do something more akin to what we do for dentry functions where we validate them on registration, and we could fix up or validate read/write pointers, with semantics something like if (!fop-write) fop-write = fop-aio_write ? do_sync_write : EINVAL_write; if (!fop-read) fop-read = fop-aio_read ? do_sync_read : EINVAL_read; kind of things? Not a big deal, perhaps. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Yet another pipe related oops.
On Wed, Mar 27, 2013 at 09:33:35AM -0700, Linus Torvalds wrote: Applied. Do we actually have files with NULL f_ops pointers? Should we? What could we possibly do with a file descriptor that doesn't have any fops? We shouldn't, at least not for something that has been successfully opened. I've a patch series cleaning that up a bit in the local queue; will check for bitrot and throw into for-next. Another thing that is a definite for-next fodder - we really have no reason to put anything non-regular or opened not for write into -s_files. And since read-only opens outnumber write-only/read-write ones by far (two orders of magnitude for something like kernel build), that gives a nice reduction of files_lglock accesses. OTOH, the only remaining user of those lists is forced remount to read-only, and I'm not at all sure we wouldn't be better off by leaving those opened files alone and just teaching file_start_write() to fail with EROFS on such fs. Then we could get rid of files_lglock and -s_files completely... Also, perhaps we should do something more akin to what we do for dentry functions where we validate them on registration, and we could fix up or validate read/write pointers, with semantics something like if (!fop-write) fop-write = fop-aio_write ? do_sync_write : EINVAL_write; if (!fop-read) fop-read = fop-aio_read ? do_sync_read : EINVAL_read; kind of things? As it is, file_operations instances are const, and it's a good idea, IMO... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
