diff mbox series

[09/10] nfsd: expose some more information about NFSv4 opens

Message ID 1556201060-7947-10-git-send-email-bfields@redhat.com (mailing list archive)
State New, archived
Headers show
Series exposing knfsd opens to userspace | expand

Commit Message

Bruce Fields April 25, 2019, 2:04 p.m. UTC 
From: "J. Bruce Fields" <bfields@redhat.com>

Add open modes, device numbers, inode numbers, and open owners to each
line of nfsd/clients/#/opens.

Open owners are totally opaque but seem to sometimes have some useful
ascii strings included, so passing through printable ascii characters
and escaping the rest seems useful while still being machine-readable.
---
 fs/nfsd/nfs4state.c            | 40 ++++++++++++++++++++++++++++------
 fs/nfsd/state.h                |  2 +-
 fs/seq_file.c                  | 17 +++++++++++++++
 include/linux/seq_file.h       |  2 ++
 include/linux/string_helpers.h |  1 +
 lib/string_helpers.c           |  5 +++--
 6 files changed, 57 insertions(+), 10 deletions(-)

Comments

Benjamin Coddington May 2, 2019, 3:28 p.m. UTC | #1 
On 25 Apr 2019, at 10:04, J. Bruce Fields wrote:

> From: "J. Bruce Fields" <bfields@redhat.com>
>
> Add open modes, device numbers, inode numbers, and open owners to each
> line of nfsd/clients/#/opens.
>
> Open owners are totally opaque but seem to sometimes have some useful
> ascii strings included, so passing through printable ascii characters
> and escaping the rest seems useful while still being machine-readable.
> ---
>  fs/nfsd/nfs4state.c            | 40 
> ++++++++++++++++++++++++++++------
>  fs/nfsd/state.h                |  2 +-
>  fs/seq_file.c                  | 17 +++++++++++++++
>  include/linux/seq_file.h       |  2 ++
>  include/linux/string_helpers.h |  1 +
>  lib/string_helpers.c           |  5 +++--
>  6 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 829d1e5440d3..f53621a65e60 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -42,6 +42,7 @@
>  #include <linux/sunrpc/svcauth_gss.h>
>  #include <linux/sunrpc/addr.h>
>  #include <linux/jhash.h>
> +#include <linux/string_helpers.h>
>  #include "xdr4.h"
>  #include "xdr4cb.h"
>  #include "vfs.h"
> @@ -2261,16 +2262,41 @@ static void opens_stop(struct seq_file *s, 
> void *v)
>  static int opens_show(struct seq_file *s, void *v)
>  {
>  	struct nfs4_stid *st = v;
> -	struct nfs4_ol_stateid *os;
> -	u64 stateid;
> +	struct nfs4_ol_stateid *ols;
> +	struct nfs4_file *nf;
> +	struct file *file;
> +	struct inode *inode;
> +	struct nfs4_stateowner *oo;
> +	unsigned int access, deny;
>
>  	if (st->sc_type != NFS4_OPEN_STID)
>  		return 0; /* XXX: or SEQ_SKIP? */
> -	os = openlockstateid(st);
> -	/* XXX: get info about file, etc., here */
> +	ols = openlockstateid(st);
> +	oo = ols->st_stateowner;
> +	nf = st->sc_file;
> +	file = find_any_file(nf);
> +	inode = d_inode(file->f_path.dentry);
> +
> +	seq_printf(s, STATEID_FMT, STATEID_VAL(&st->sc_stateid));

Should we match the byte order printed with what wireshark/tshark sees?

For example, this will print:

5ccb016e/6d028c97/00000002/00000002 -w  --  fd:00:9163439   'open 
id:\x00\x00\x00.\x00\x00\x00\x00\x00\x00\x021\x8dp\xbe\xc7'

But, tshark -V prints:

         Opcode: OPEN (18)
             Status: NFS4_OK (0)
             stateid
                 [StateID Hash: 0x8298]
                 seqid: 0x00000002
                 Data: 6e01cb5c978c026d02000000
                 [Data hash (CRC-32): 0x8391069f]

I think this is the first time we've exposed state ids to users from 
knfsd,
I wonder if we should make it easier for everyone that might want to try 
to
correlate this information with what they can see in a packet capture.

Ben
Andrew W Elble May 2, 2019, 3:58 p.m. UTC | #2 
Benjamin Coddington <bcodding@redhat.com> writes:

> On 25 Apr 2019, at 10:04, J. Bruce Fields wrote:
>
>> From: "J. Bruce Fields" <bfields@redhat.com>
>>
>> Add open modes, device numbers, inode numbers, and open owners to each
>> line of nfsd/clients/#/opens.
>>
>> Open owners are totally opaque but seem to sometimes have some useful
>> ascii strings included, so passing through printable ascii characters
>> and escaping the rest seems useful while still being machine-readable.
>> ---
>>  fs/nfsd/nfs4state.c            | 40
>> ++++++++++++++++++++++++++++------
>>  fs/nfsd/state.h                |  2 +-
>>  fs/seq_file.c                  | 17 +++++++++++++++
>>  include/linux/seq_file.h       |  2 ++
>>  include/linux/string_helpers.h |  1 +
>>  lib/string_helpers.c           |  5 +++--
>>  6 files changed, 57 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 829d1e5440d3..f53621a65e60 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -42,6 +42,7 @@
>>  #include <linux/sunrpc/svcauth_gss.h>
>>  #include <linux/sunrpc/addr.h>
>>  #include <linux/jhash.h>
>> +#include <linux/string_helpers.h>
>>  #include "xdr4.h"
>>  #include "xdr4cb.h"
>>  #include "vfs.h"
>> @@ -2261,16 +2262,41 @@ static void opens_stop(struct seq_file *s,
>> void *v)
>>  static int opens_show(struct seq_file *s, void *v)
>>  {
>>  	struct nfs4_stid *st = v;
>> -	struct nfs4_ol_stateid *os;
>> -	u64 stateid;
>> +	struct nfs4_ol_stateid *ols;
>> +	struct nfs4_file *nf;
>> +	struct file *file;
>> +	struct inode *inode;
>> +	struct nfs4_stateowner *oo;
>> +	unsigned int access, deny;
>>
>>  	if (st->sc_type != NFS4_OPEN_STID)
>>  		return 0; /* XXX: or SEQ_SKIP? */
>> -	os = openlockstateid(st);
>> -	/* XXX: get info about file, etc., here */
>> +	ols = openlockstateid(st);
>> +	oo = ols->st_stateowner;
>> +	nf = st->sc_file;
>> +	file = find_any_file(nf);

Is there a matching fput() missing somewhere, or did I just not see it...?

>> +	inode = d_inode(file->f_path.dentry);
>> +
>> +	seq_printf(s, STATEID_FMT, STATEID_VAL(&st->sc_stateid));
>
> Should we match the byte order printed with what wireshark/tshark sees?

^^ +1


Thanks,

Andy
J. Bruce Fields May 7, 2019, 1:02 a.m. UTC | #3 
On Thu, May 02, 2019 at 11:58:06AM -0400, Andrew W Elble wrote:
> Benjamin Coddington <bcodding@redhat.com> writes:
> 
> > On 25 Apr 2019, at 10:04, J. Bruce Fields wrote:
> >
> >> From: "J. Bruce Fields" <bfields@redhat.com>
> >>
> >> Add open modes, device numbers, inode numbers, and open owners to each
> >> line of nfsd/clients/#/opens.
> >>
> >> Open owners are totally opaque but seem to sometimes have some useful
> >> ascii strings included, so passing through printable ascii characters
> >> and escaping the rest seems useful while still being machine-readable.
> >> ---
> >>  fs/nfsd/nfs4state.c            | 40
> >> ++++++++++++++++++++++++++++------
> >>  fs/nfsd/state.h                |  2 +-
> >>  fs/seq_file.c                  | 17 +++++++++++++++
> >>  include/linux/seq_file.h       |  2 ++
> >>  include/linux/string_helpers.h |  1 +
> >>  lib/string_helpers.c           |  5 +++--
> >>  6 files changed, 57 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 829d1e5440d3..f53621a65e60 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -42,6 +42,7 @@
> >>  #include <linux/sunrpc/svcauth_gss.h>
> >>  #include <linux/sunrpc/addr.h>
> >>  #include <linux/jhash.h>
> >> +#include <linux/string_helpers.h>
> >>  #include "xdr4.h"
> >>  #include "xdr4cb.h"
> >>  #include "vfs.h"
> >> @@ -2261,16 +2262,41 @@ static void opens_stop(struct seq_file *s,
> >> void *v)
> >>  static int opens_show(struct seq_file *s, void *v)
> >>  {
> >>  	struct nfs4_stid *st = v;
> >> -	struct nfs4_ol_stateid *os;
> >> -	u64 stateid;
> >> +	struct nfs4_ol_stateid *ols;
> >> +	struct nfs4_file *nf;
> >> +	struct file *file;
> >> +	struct inode *inode;
> >> +	struct nfs4_stateowner *oo;
> >> +	unsigned int access, deny;
> >>
> >>  	if (st->sc_type != NFS4_OPEN_STID)
> >>  		return 0; /* XXX: or SEQ_SKIP? */
> >> -	os = openlockstateid(st);
> >> -	/* XXX: get info about file, etc., here */
> >> +	ols = openlockstateid(st);
> >> +	oo = ols->st_stateowner;
> >> +	nf = st->sc_file;
> >> +	file = find_any_file(nf);
> 
> Is there a matching fput() missing somewhere, or did I just not see it...?

Oops, fixed.

> >> +	inode = d_inode(file->f_path.dentry);
> >> +
> >> +	seq_printf(s, STATEID_FMT, STATEID_VAL(&st->sc_stateid));
> >
> > Should we match the byte order printed with what wireshark/tshark sees?
> 
> ^^ +1

Yeah, I agree, I'm changing that to just be a "%16phN", which should
give us what wireshark does.

Thanks for the review!

--b.
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 829d1e5440d3..f53621a65e60 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -42,6 +42,7 @@ 
 #include <linux/sunrpc/svcauth_gss.h>
 #include <linux/sunrpc/addr.h>
 #include <linux/jhash.h>
+#include <linux/string_helpers.h>
 #include "xdr4.h"
 #include "xdr4cb.h"
 #include "vfs.h"
@@ -2261,16 +2262,41 @@  static void opens_stop(struct seq_file *s, void *v)
 static int opens_show(struct seq_file *s, void *v)
 {
 	struct nfs4_stid *st = v;
-	struct nfs4_ol_stateid *os;
-	u64 stateid;
+	struct nfs4_ol_stateid *ols;
+	struct nfs4_file *nf;
+	struct file *file;
+	struct inode *inode;
+	struct nfs4_stateowner *oo;
+	unsigned int access, deny;
 
 	if (st->sc_type != NFS4_OPEN_STID)
 		return 0; /* XXX: or SEQ_SKIP? */
-	os = openlockstateid(st);
-	/* XXX: get info about file, etc., here */
+	ols = openlockstateid(st);
+	oo = ols->st_stateowner;
+	nf = st->sc_file;
+	file = find_any_file(nf);
+	inode = d_inode(file->f_path.dentry);
+
+	seq_printf(s, STATEID_FMT, STATEID_VAL(&st->sc_stateid));
+
+	access = bmap_to_share_mode(ols->st_access_bmap);
+	deny   = bmap_to_share_mode(ols->st_deny_bmap);
+
+	seq_printf(s, "\t\%s\%s",
+		access & NFS4_SHARE_ACCESS_READ ? "r" : "-",
+		access & NFS4_SHARE_ACCESS_WRITE ? "w" : "-");
+	seq_printf(s, "\t\%s\%s",
+		deny & NFS4_SHARE_ACCESS_READ ? "R" : "-",
+		deny & NFS4_SHARE_ACCESS_WRITE ? "W" : "-");
+
+	seq_printf(s, "\t%02x:%02x:%ld\t", MAJOR(inode->i_sb->s_dev),
+					 MINOR(inode->i_sb->s_dev),
+					 inode->i_ino);
+	seq_printf(s, "'");
+	seq_escape_mem(s, oo->so_owner.data, oo->so_owner.len,
+			ESCAPE_NP|ESCAPE_HEX|ESCAPE_NOHIGH, NULL);
+	seq_printf(s, "'\n");
 
-	memcpy(&stateid, &st->sc_stateid, sizeof(stateid));
-	seq_printf(s, "stateid: %llx\n", stateid);
 	return 0;
 }
 
@@ -2320,7 +2346,7 @@  static const struct file_operations client_opens_fops = {
 
 static const struct tree_descr client_files[] = {
 	[0] = {"info", &client_info_fops, S_IRUSR},
-	[1] = {"open", &client_opens_fops, S_IRUSR},
+	[1] = {"opens", &client_opens_fops, S_IRUSR},
 	[2] = {""},
 };
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 5a999e4b766e..4db22433be82 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -56,7 +56,7 @@  typedef struct {
 	stateid_opaque_t        si_opaque;
 } stateid_t;
 
-#define STATEID_FMT	"(%08x/%08x/%08x/%08x)"
+#define STATEID_FMT	"%08x/%08x/%08x/%08x"
 #define STATEID_VAL(s) \
 	(s)->si_opaque.so_clid.cl_boot, \
 	(s)->si_opaque.so_clid.cl_id, \
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1dea7a8a5255..bb646e0c5b9c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -383,6 +383,23 @@  void seq_escape(struct seq_file *m, const char *s, const char *esc)
 }
 EXPORT_SYMBOL(seq_escape);
 
+/**
+ * seq_escape_mem - copy data to buffer, escaping some characters
+ *
+ * See string_escape_mem for explanations of the arguments.
+ */
+void seq_escape_mem(struct seq_file *m, const char *src, size_t isz,
+		    unsigned int flags, const char *only)
+{
+	char *buf;
+	size_t size = seq_get_buf(m, &buf);
+	int ret;
+
+	ret = string_escape_mem(src, isz, buf, size, flags, only);
+	seq_commit(m, ret < size ? ret : -1);
+}
+EXPORT_SYMBOL(seq_escape_mem);
+
 void seq_vprintf(struct seq_file *m, const char *f, va_list args)
 {
 	int len;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index a121982af0f5..de6ace9e5e6b 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -127,6 +127,8 @@  void seq_put_hex_ll(struct seq_file *m, const char *delimiter,
 		    unsigned long long v, unsigned int width);
 
 void seq_escape(struct seq_file *m, const char *s, const char *esc);
+void seq_escape_mem(struct seq_file *m, const char *src, size_t isz,
+		    unsigned int flags, const char *only);
 
 void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
 		  int rowsize, int groupsize, const void *buf, size_t len,
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index d23c5030901a..54034526aa46 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -50,6 +50,7 @@  static inline int string_unescape_any_inplace(char *buf)
 #define ESCAPE_NP		0x10
 #define ESCAPE_ANY_NP		(ESCAPE_ANY | ESCAPE_NP)
 #define ESCAPE_HEX		0x20
+#define ESCAPE_NOHIGH		0x40
 
 int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 		unsigned int flags, const char *only);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 29c490e5d478..2c3e7b93775e 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -511,8 +511,9 @@  int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 		 * In these cases we just pass through a character to the
 		 * output buffer.
 		 */
-		if ((flags & ESCAPE_NP && isprint(c)) ||
-		    (is_dict && !strchr(only, c))) {
+		if ((  (flags & ESCAPE_NP && isprint(c)) ||
+		       (is_dict && !strchr(only, c))) &&
+		    !(flags & ESCAPE_NOHIGH && (unsigned char)c > 127)) {
 			/* do nothing */
 		} else {
 			if (flags & ESCAPE_SPACE && escape_space(c, &p, end))