Re: [PATCH RFC] proc: Don't retain negative dentries
Hello,
Thanks very much for the elaboration!
On 8/10/18 20:01, Al Viro wrote:
> On Mon, Oct 08, 2018 at 07:02:09PM +0200, Ahmad Fatoum wrote:
> OK, to elaborate: where have you seen negative dentries on procfs in
> the first place? I'm trying to find a way for such to happen, but
> I don't see any. And in any case, these ->d_delete() and ->d_revalidate()
> instances would've been oopsing on such.
Sorry for the misunderstanding on my part. The referenced commit replaced
simple_dentry_operations whose only non-NULL member points at:
/*
* Retaining negative dentries for an in-memory filesystem just wastes
* memory and lookup time: arrange for them to be deleted immediately.
*/
int always_delete_dentry(const struct dentry *dentry)
{
return 1;
}
which is what I based my rationale on.
> ->d_delete() is about retaining _unused_ dentries in hash for future lookups;
> nothing to do with positive/negative. *And* ->d_delete() is called only when
> refcount hits zero. If another process opens the damn thing and keeps it
> opened,
> ->d_delete() won't be called at all and your patch won't change the behaviour
> of the entire thing.
I see. Would that mean that previously it only worked by chance?
> If anything, you might want to have separate ->d_op for /proc/*/net, so
> that its ->d_revalidate() would return 0 if netns doesn't match. Would
> need a way to keep some information allowing to detect the switchover, of
> course (either in PROC_I(inode) or in ->d_fsdata of that dentry - in the
> latter case you'd want to do whatever you need to dispose of that in
> ->d_release()). Check in revalidate should be along the lines of "do what's
> currently done in get_proc_task_net(), compare the result with the memorized
> value, bugger off on mismatch", perhaps with memorized value being
> counted as a reference (in which case you'd want to do put_net() when
> disposing of the inode or dentry, whichever you use to keep it in). In
> that case
> proc_tgid_net_lookup()/proc_tgid_net_getattr()/proc_tgid_net_readdir()
> would simply use the stored reference instead of messing with
> get_proc_task_net()
> and put_net().
>
> You'd need separate dentry_operations for /proc/*/net and /proc/*/*/net,
> instead of using pid_dentry_operations. That would need to be recognized
> in proc_pident_instantiate() (_without_ memcmp(p->name, "net", 4) on each
> call of that thing, preferably). I'd put that new instance of
> dentry_operations
> (along with the methods in it, of course) into fs/proc/proc_net.c, where
> we already have the inode and file methods of /proc/*/net.
>
I will need to understand why my patch seemed to fix it and would try to
find some time later this month to implement it properly (or if someone
finds that time now, I would appreciate that too!)
Thanks again
Ahmad
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH RFC] proc: Don't retain negative dentries
Hello,
Thanks very much for the elaboration!
On 8/10/18 20:01, Al Viro wrote:
> On Mon, Oct 08, 2018 at 07:02:09PM +0200, Ahmad Fatoum wrote:
> OK, to elaborate: where have you seen negative dentries on procfs in
> the first place? I'm trying to find a way for such to happen, but
> I don't see any. And in any case, these ->d_delete() and ->d_revalidate()
> instances would've been oopsing on such.
Sorry for the misunderstanding on my part. The referenced commit replaced
simple_dentry_operations whose only non-NULL member points at:
/*
* Retaining negative dentries for an in-memory filesystem just wastes
* memory and lookup time: arrange for them to be deleted immediately.
*/
int always_delete_dentry(const struct dentry *dentry)
{
return 1;
}
which is what I based my rationale on.
> ->d_delete() is about retaining _unused_ dentries in hash for future lookups;
> nothing to do with positive/negative. *And* ->d_delete() is called only when
> refcount hits zero. If another process opens the damn thing and keeps it
> opened,
> ->d_delete() won't be called at all and your patch won't change the behaviour
> of the entire thing.
I see. Would that mean that previously it only worked by chance?
> If anything, you might want to have separate ->d_op for /proc/*/net, so
> that its ->d_revalidate() would return 0 if netns doesn't match. Would
> need a way to keep some information allowing to detect the switchover, of
> course (either in PROC_I(inode) or in ->d_fsdata of that dentry - in the
> latter case you'd want to do whatever you need to dispose of that in
> ->d_release()). Check in revalidate should be along the lines of "do what's
> currently done in get_proc_task_net(), compare the result with the memorized
> value, bugger off on mismatch", perhaps with memorized value being
> counted as a reference (in which case you'd want to do put_net() when
> disposing of the inode or dentry, whichever you use to keep it in). In
> that case
> proc_tgid_net_lookup()/proc_tgid_net_getattr()/proc_tgid_net_readdir()
> would simply use the stored reference instead of messing with
> get_proc_task_net()
> and put_net().
>
> You'd need separate dentry_operations for /proc/*/net and /proc/*/*/net,
> instead of using pid_dentry_operations. That would need to be recognized
> in proc_pident_instantiate() (_without_ memcmp(p->name, "net", 4) on each
> call of that thing, preferably). I'd put that new instance of
> dentry_operations
> (along with the methods in it, of course) into fs/proc/proc_net.c, where
> we already have the inode and file methods of /proc/*/net.
>
I will need to understand why my patch seemed to fix it and would try to
find some time later this month to implement it properly (or if someone
finds that time now, I would appreciate that too!)
Thanks again
Ahmad
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH RFC] proc: Don't retain negative dentries
On Mon, Oct 08, 2018 at 07:02:09PM +0200, Ahmad Fatoum wrote: > Hello, > > On 10/8/18 6:55 PM, Al Viro wrote: > > > > What the hell does that have to do with negative dentries anywhere??? > > It's possible that this needs fixing at another place. I don't know, > but this seems to work for me, that's why I prefixed with RFC. OK, to elaborate: where have you seen negative dentries on procfs in the first place? I'm trying to find a way for such to happen, but I don't see any. And in any case, these ->d_delete() and ->d_revalidate() instances would've been oopsing on such. ->d_delete() is about retaining _unused_ dentries in hash for future lookups; nothing to do with positive/negative. *And* ->d_delete() is called only when refcount hits zero. If another process opens the damn thing and keeps it opened, ->d_delete() won't be called at all and your patch won't change the behaviour of the entire thing. If anything, you might want to have separate ->d_op for /proc/*/net, so that its ->d_revalidate() would return 0 if netns doesn't match. Would need a way to keep some information allowing to detect the switchover, of course (either in PROC_I(inode) or in ->d_fsdata of that dentry - in the latter case you'd want to do whatever you need to dispose of that in ->d_release()). Check in revalidate should be along the lines of "do what's currently done in get_proc_task_net(), compare the result with the memorized value, bugger off on mismatch", perhaps with memorized value being counted as a reference (in which case you'd want to do put_net() when disposing of the inode or dentry, whichever you use to keep it in). In that case proc_tgid_net_lookup()/proc_tgid_net_getattr()/proc_tgid_net_readdir() would simply use the stored reference instead of messing with get_proc_task_net() and put_net(). You'd need separate dentry_operations for /proc/*/net and /proc/*/*/net, instead of using pid_dentry_operations. That would need to be recognized in proc_pident_instantiate() (_without_ memcmp(p->name, "net", 4) on each call of that thing, preferably). I'd put that new instance of dentry_operations (along with the methods in it, of course) into fs/proc/proc_net.c, where we already have the inode and file methods of /proc/*/net.
Re: [PATCH RFC] proc: Don't retain negative dentries
On Mon, Oct 08, 2018 at 07:02:09PM +0200, Ahmad Fatoum wrote: > Hello, > > On 10/8/18 6:55 PM, Al Viro wrote: > > > > What the hell does that have to do with negative dentries anywhere??? > > It's possible that this needs fixing at another place. I don't know, > but this seems to work for me, that's why I prefixed with RFC. OK, to elaborate: where have you seen negative dentries on procfs in the first place? I'm trying to find a way for such to happen, but I don't see any. And in any case, these ->d_delete() and ->d_revalidate() instances would've been oopsing on such. ->d_delete() is about retaining _unused_ dentries in hash for future lookups; nothing to do with positive/negative. *And* ->d_delete() is called only when refcount hits zero. If another process opens the damn thing and keeps it opened, ->d_delete() won't be called at all and your patch won't change the behaviour of the entire thing. If anything, you might want to have separate ->d_op for /proc/*/net, so that its ->d_revalidate() would return 0 if netns doesn't match. Would need a way to keep some information allowing to detect the switchover, of course (either in PROC_I(inode) or in ->d_fsdata of that dentry - in the latter case you'd want to do whatever you need to dispose of that in ->d_release()). Check in revalidate should be along the lines of "do what's currently done in get_proc_task_net(), compare the result with the memorized value, bugger off on mismatch", perhaps with memorized value being counted as a reference (in which case you'd want to do put_net() when disposing of the inode or dentry, whichever you use to keep it in). In that case proc_tgid_net_lookup()/proc_tgid_net_getattr()/proc_tgid_net_readdir() would simply use the stored reference instead of messing with get_proc_task_net() and put_net(). You'd need separate dentry_operations for /proc/*/net and /proc/*/*/net, instead of using pid_dentry_operations. That would need to be recognized in proc_pident_instantiate() (_without_ memcmp(p->name, "net", 4) on each call of that thing, preferably). I'd put that new instance of dentry_operations (along with the methods in it, of course) into fs/proc/proc_net.c, where we already have the inode and file methods of /proc/*/net.
Re: [PATCH RFC] proc: Don't retain negative dentries
Hello, On 10/8/18 6:55 PM, Al Viro wrote: > > What the hell does that have to do with negative dentries anywhere??? It's possible that this needs fixing at another place. I don't know, but this seems to work for me, that's why I prefixed with RFC. > NAK. Rationale makes no sense _and_ the patch has nothing to do > with claimed rationale. Please CC me when this gets fixed properly. I am curious. :-) Cheers Ahmad
Re: [PATCH RFC] proc: Don't retain negative dentries
Hello, On 10/8/18 6:55 PM, Al Viro wrote: > > What the hell does that have to do with negative dentries anywhere??? It's possible that this needs fixing at another place. I don't know, but this seems to work for me, that's why I prefixed with RFC. > NAK. Rationale makes no sense _and_ the patch has nothing to do > with claimed rationale. Please CC me when this gets fixed properly. I am curious. :-) Cheers Ahmad
Re: [PATCH RFC] proc: Don't retain negative dentries
On Mon, Oct 08, 2018 at 06:50:10PM +0200, Ahmad Fatoum wrote:
> The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
> caused following userspace code to access a stale /proc/net/dev
> after the network namespace was changed:
> Despite switching the network namespace, the read access from the
> newly opened file gave back what the very first read in the default
> network namespace returned.
What the hell does that have to do with negative dentries anywhere???
NAK. Rationale makes no sense _and_ the patch has nothing to do
with claimed rationale.
Re: [PATCH RFC] proc: Don't retain negative dentries
On Mon, Oct 08, 2018 at 06:50:10PM +0200, Ahmad Fatoum wrote:
> The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
> caused following userspace code to access a stale /proc/net/dev
> after the network namespace was changed:
> Despite switching the network namespace, the read access from the
> newly opened file gave back what the very first read in the default
> network namespace returned.
What the hell does that have to do with negative dentries anywhere???
NAK. Rationale makes no sense _and_ the patch has nothing to do
with claimed rationale.
Re: [PATCH RFC] proc: Don't retain negative dentries
On 10/8/18 6:50 PM, Ahmad Fatoum wrote:
> The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
> caused following userspace code to access a stale /proc/net/dev
> after the network namespace was changed:
>
> system("ip netns add testns");
>
> printf("default:\n"); {
> int devinfd = open("/proc/net/dev", O_RDONLY);
> sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
> close(devinfd);
> }
>
> printf("testns:\n"); {
> int ns_fd = open("/var/run/netns/testns", O_RDONLY);
> setns(ns_fd, 0);
>
> int devinfd = open("/proc/net/dev", O_RDONLY);
> sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
> close(devinfd);
>
> close(ns_fd);
> }
>
> Despite switching the network namespace, the read access from the
> newly opened file gave back what the very first read in the default
> network namespace returned.
>
> This doesn't occur if /proc/net/dev is opened within a new process,
> which might explain why this wasn't noticed previously.
>
> As I don't see a reason why one would keep negative dentries for procfs
> at all, amend the code not to do this anymore.
>
> Fixes: 1da4d377f94 ("proc: Don't retain negative dentries")
This should read
Fixes: 1da4d377f94 ("proc: revalidate misc dentries")
instead...
Re: [PATCH RFC] proc: Don't retain negative dentries
On 10/8/18 6:50 PM, Ahmad Fatoum wrote:
> The referenced commit 1da4d377f94 ("proc: revalidate misc dentries")
> caused following userspace code to access a stale /proc/net/dev
> after the network namespace was changed:
>
> system("ip netns add testns");
>
> printf("default:\n"); {
> int devinfd = open("/proc/net/dev", O_RDONLY);
> sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
> close(devinfd);
> }
>
> printf("testns:\n"); {
> int ns_fd = open("/var/run/netns/testns", O_RDONLY);
> setns(ns_fd, 0);
>
> int devinfd = open("/proc/net/dev", O_RDONLY);
> sendfile(STDOUT_FILENO, devinfd, NULL, 4096);
> close(devinfd);
>
> close(ns_fd);
> }
>
> Despite switching the network namespace, the read access from the
> newly opened file gave back what the very first read in the default
> network namespace returned.
>
> This doesn't occur if /proc/net/dev is opened within a new process,
> which might explain why this wasn't noticed previously.
>
> As I don't see a reason why one would keep negative dentries for procfs
> at all, amend the code not to do this anymore.
>
> Fixes: 1da4d377f94 ("proc: Don't retain negative dentries")
This should read
Fixes: 1da4d377f94 ("proc: revalidate misc dentries")
instead...
