| Message ID | 20171024214814.GO5343@uranus |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] net/unix_diag: Provide UDIAG_SHOW_VFS2 attribute to fetch complete inode number | expand |
On Wed, Oct 25, 2017 at 12:48:14AM +0300, Cyrill Gorcunov wrote: > Currently unix_diag_vfs structure reports unix socket inode > as u32 value which of course doesn't fit to ino_t type and BTW: As far as I understand, it is not a problem right now, because get_next_ino returns int. And I'm agree that it maybe a problem in a future and it is better to be ready. > the number may be trimmed. Lets rather deprecate old UDIAG_SHOW_VFS > interface and provide UDIAG_SHOW_VFS2 (with one field "__zero" reserved > which we could extend in future). There is one more place where we return ino as u32: static int sk_diag_dump_peer(struct sock *sk, struct sk_buff *nlskb) .... return nla_put_u32(nlskb, UNIX_DIAG_PEER, ino); > > CC: Andrey Vagin <avagin@openvz.org> > CC: David S. Miller <davem@davemloft.net> > CC: Pavel Emelyanov <xemul@virtuozzo.com> > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org> > --- > > I build-tested it only thus not for inclusion yet, but rather > to discuss if there some better way to handle this potential > problem. > > include/uapi/linux/unix_diag.h | 8 ++++++++ > net/unix/diag.c | 25 ++++++++++++++++--------- > 2 files changed, 24 insertions(+), 9 deletions(-) > > Index: linux-ml.git/include/uapi/linux/unix_diag.h > =================================================================== > --- linux-ml.git.orig/include/uapi/linux/unix_diag.h > +++ linux-ml.git/include/uapi/linux/unix_diag.h > @@ -19,6 +19,7 @@ struct unix_diag_req { > #define UDIAG_SHOW_ICONS 0x00000008 /* show pending connections */ > #define UDIAG_SHOW_RQLEN 0x00000010 /* show skb receive queue len */ > #define UDIAG_SHOW_MEMINFO 0x00000020 /* show memory info of a socket */ > +#define UDIAG_SHOW_VFS2 0x00000040 /* show VFS inode info v2 */ > > struct unix_diag_msg { > __u8 udiag_family; > @@ -39,6 +40,7 @@ enum { > UNIX_DIAG_RQLEN, > UNIX_DIAG_MEMINFO, > UNIX_DIAG_SHUTDOWN, > + UNIX_DIAG_VFS2, > > __UNIX_DIAG_MAX, > }; > @@ -50,6 +52,12 @@ struct unix_diag_vfs { > __u32 udiag_vfs_dev; > }; > > +struct unix_diag_vfs2 { > + __u64 udiag_vfs_ino; > + __u32 udiag_vfs_dev; > + __u32 __zero; /* Reserve for future use */ How can a user understand whether this field is used or not? Each netlink attribute has its size in a header. Any attribute can be extended, and users can understand which fields are filled by a size of an attribute. > +}; > + > struct unix_diag_rqlen { > __u32 udiag_rqueue; > __u32 udiag_wqueue; > Index: linux-ml.git/net/unix/diag.c > =================================================================== > --- linux-ml.git.orig/net/unix/diag.c > +++ linux-ml.git/net/unix/diag.c > @@ -19,17 +19,24 @@ static int sk_diag_dump_name(struct sock > addr->name->sun_path); > } > > -static int sk_diag_dump_vfs(struct sock *sk, struct sk_buff *nlskb) > +static int sk_diag_dump_vfs(struct sock *sk, struct sk_buff *nlskb, unsigned int flags) > { > struct dentry *dentry = unix_sk(sk)->path.dentry; > > if (dentry) { > - struct unix_diag_vfs uv = { > - .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, > - .udiag_vfs_dev = dentry->d_sb->s_dev, > - }; > - > - return nla_put(nlskb, UNIX_DIAG_VFS, sizeof(uv), &uv); > + if (flags & UDIAG_SHOW_VFS2) { > + struct unix_diag_vfs uv = { > + .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, > + .udiag_vfs_dev = dentry->d_sb->s_dev, > + }; > + return nla_put(nlskb, UNIX_DIAG_VFS, sizeof(uv), &uv); > + } else { > + struct unix_diag_vfs2 uv = { > + .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, > + .udiag_vfs_dev = dentry->d_sb->s_dev, > + }; > + return nla_put(nlskb, UDIAG_SHOW_VFS2, sizeof(uv), &uv); > + } > } > > return 0; > @@ -132,8 +139,8 @@ static int sk_diag_fill(struct sock *sk, > sk_diag_dump_name(sk, skb)) > goto out_nlmsg_trim; > > - if ((req->udiag_show & UDIAG_SHOW_VFS) && > - sk_diag_dump_vfs(sk, skb)) > + if ((req->udiag_show & (UDIAG_SHOW_VFS | UDIAG_SHOW_VFS2)) && > + sk_diag_dump_vfs(sk, skb, req->udiag_show)) > goto out_nlmsg_trim; > > if ((req->udiag_show & UDIAG_SHOW_PEER) &&
On Tue, Oct 24, 2017 at 05:25:16PM -0700, Andrei Vagin wrote: > On Wed, Oct 25, 2017 at 12:48:14AM +0300, Cyrill Gorcunov wrote: > > Currently unix_diag_vfs structure reports unix socket inode > > as u32 value which of course doesn't fit to ino_t type and > > BTW: As far as I understand, it is not a problem right now, because > get_next_ino returns int. And I'm agree that it maybe a problem in a > future and it is better to be ready. > > > the number may be trimmed. Lets rather deprecate old UDIAG_SHOW_VFS > > interface and provide UDIAG_SHOW_VFS2 (with one field "__zero" reserved > > which we could extend in future). > > There is one more place where we return ino as u32: > > static int sk_diag_dump_peer(struct sock *sk, struct sk_buff *nlskb) > .... > return nla_put_u32(nlskb, UNIX_DIAG_PEER, ino); Managed to miss it, thanks! > > +struct unix_diag_vfs2 { > > + __u64 udiag_vfs_ino; > > + __u32 udiag_vfs_dev; > > + __u32 __zero; /* Reserve for future use */ > > How can a user understand whether this field is used or not? Checking out if it zero or not. > Each netlink attribute has its size in a header. Any attribute can be > extended, and users can understand which fields are filled by > a size of an attribute. Well, that's correct, but it implies that any extension has different size. I though of extending this structure (if ever needed) the way that same attribute may carry different structures equal in size and setting up @__zero field with some bit would help. On the other side it become more complex than needed, so now I think I should simply drop __zero out. Thanks for comments, Andrew! Cyrill
Cyrill Gorcunov <gorcunov@gmail.com> writes: > Currently unix_diag_vfs structure reports unix socket inode > as u32 value which of course doesn't fit to ino_t type and > the number may be trimmed. Lets rather deprecate old UDIAG_SHOW_VFS > interface and provide UDIAG_SHOW_VFS2 (with one field "__zero" reserved > which we could extend in future). > [...] > -static int sk_diag_dump_vfs(struct sock *sk, struct sk_buff *nlskb) > +static int sk_diag_dump_vfs(struct sock *sk, struct sk_buff *nlskb, unsigned int flags) > { > struct dentry *dentry = unix_sk(sk)->path.dentry; > > if (dentry) { > - struct unix_diag_vfs uv = { > - .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, > - .udiag_vfs_dev = dentry->d_sb->s_dev, > - }; > - > - return nla_put(nlskb, UNIX_DIAG_VFS, sizeof(uv), &uv); > + if (flags & UDIAG_SHOW_VFS2) { > + struct unix_diag_vfs uv = { > + .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, > + .udiag_vfs_dev = dentry->d_sb->s_dev, > + }; > + return nla_put(nlskb, UNIX_DIAG_VFS, sizeof(uv), &uv); > + } else { > + struct unix_diag_vfs2 uv = { > + .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, > + .udiag_vfs_dev = dentry->d_sb->s_dev, > + }; I think __zero should be explicitly set to 0. [...]
On Wed, Oct 25, 2017 at 09:55:07AM -0400, Roman Mashak wrote: > > I think __zero should be explicitly set to 0. It will be by compiler default.
Index: linux-ml.git/include/uapi/linux/unix_diag.h =================================================================== --- linux-ml.git.orig/include/uapi/linux/unix_diag.h +++ linux-ml.git/include/uapi/linux/unix_diag.h @@ -19,6 +19,7 @@ struct unix_diag_req { #define UDIAG_SHOW_ICONS 0x00000008 /* show pending connections */ #define UDIAG_SHOW_RQLEN 0x00000010 /* show skb receive queue len */ #define UDIAG_SHOW_MEMINFO 0x00000020 /* show memory info of a socket */ +#define UDIAG_SHOW_VFS2 0x00000040 /* show VFS inode info v2 */ struct unix_diag_msg { __u8 udiag_family; @@ -39,6 +40,7 @@ enum { UNIX_DIAG_RQLEN, UNIX_DIAG_MEMINFO, UNIX_DIAG_SHUTDOWN, + UNIX_DIAG_VFS2, __UNIX_DIAG_MAX, }; @@ -50,6 +52,12 @@ struct unix_diag_vfs { __u32 udiag_vfs_dev; }; +struct unix_diag_vfs2 { + __u64 udiag_vfs_ino; + __u32 udiag_vfs_dev; + __u32 __zero; /* Reserve for future use */ +}; + struct unix_diag_rqlen { __u32 udiag_rqueue; __u32 udiag_wqueue; Index: linux-ml.git/net/unix/diag.c =================================================================== --- linux-ml.git.orig/net/unix/diag.c +++ linux-ml.git/net/unix/diag.c @@ -19,17 +19,24 @@ static int sk_diag_dump_name(struct sock addr->name->sun_path); } -static int sk_diag_dump_vfs(struct sock *sk, struct sk_buff *nlskb) +static int sk_diag_dump_vfs(struct sock *sk, struct sk_buff *nlskb, unsigned int flags) { struct dentry *dentry = unix_sk(sk)->path.dentry; if (dentry) { - struct unix_diag_vfs uv = { - .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, - .udiag_vfs_dev = dentry->d_sb->s_dev, - }; - - return nla_put(nlskb, UNIX_DIAG_VFS, sizeof(uv), &uv); + if (flags & UDIAG_SHOW_VFS2) { + struct unix_diag_vfs uv = { + .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, + .udiag_vfs_dev = dentry->d_sb->s_dev, + }; + return nla_put(nlskb, UNIX_DIAG_VFS, sizeof(uv), &uv); + } else { + struct unix_diag_vfs2 uv = { + .udiag_vfs_ino = d_backing_inode(dentry)->i_ino, + .udiag_vfs_dev = dentry->d_sb->s_dev, + }; + return nla_put(nlskb, UDIAG_SHOW_VFS2, sizeof(uv), &uv); + } } return 0; @@ -132,8 +139,8 @@ static int sk_diag_fill(struct sock *sk, sk_diag_dump_name(sk, skb)) goto out_nlmsg_trim; - if ((req->udiag_show & UDIAG_SHOW_VFS) && - sk_diag_dump_vfs(sk, skb)) + if ((req->udiag_show & (UDIAG_SHOW_VFS | UDIAG_SHOW_VFS2)) && + sk_diag_dump_vfs(sk, skb, req->udiag_show)) goto out_nlmsg_trim; if ((req->udiag_show & UDIAG_SHOW_PEER) &&
Currently unix_diag_vfs structure reports unix socket inode as u32 value which of course doesn't fit to ino_t type and the number may be trimmed. Lets rather deprecate old UDIAG_SHOW_VFS interface and provide UDIAG_SHOW_VFS2 (with one field "__zero" reserved which we could extend in future). CC: Andrey Vagin <avagin@openvz.org> CC: David S. Miller <davem@davemloft.net> CC: Pavel Emelyanov <xemul@virtuozzo.com> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org> --- I build-tested it only thus not for inclusion yet, but rather to discuss if there some better way to handle this potential problem. include/uapi/linux/unix_diag.h | 8 ++++++++ net/unix/diag.c | 25 ++++++++++++++++--------- 2 files changed, 24 insertions(+), 9 deletions(-)