mbox series

[v2,0/2] Fix sockaddr handling in NFSD trace points

Message ID 164182978641.8391.8277203495236105391.stgit@bazille.1015granger.net (mailing list archive)
Headers show
Series Fix sockaddr handling in NFSD trace points | expand

Message

Chuck Lever III Jan. 10, 2022, 3:56 p.m. UTC
The patches in this series address a simple buffer over-read bug in
the Linux NFS server.

However I was thinking it would be nice to have trace helpers to
deal safely with generic socket addresses. I'd like to be able to
treat them the same way we currently treat strings. So for example:


#define field_sockaddr(field, len)  __dynamic_array(u8, field, len)
#define assign_sockaddr(dest, src, len)  memcpy(__get_dynamic_array(dest), src, len)
#define __get_sockaddr(field)  ((struct sockaddr *)__get_dynamic_array(field))

TRACE_EVENT(sockaddr_example,
        TP_PROTO(
                const struct sockaddr *sap,
                size_t salen
        ),  
        TP_ARGS(sap, salen),
        TP_STRUCT__entry(
                __field_sockaddr(addr, salen)
        ),  
        TP_fast_assign(
                __assign_sockaddr(addr, sap, salen);
        ),  
        TP_printk("addr=%pIS", __get_sockaddr(addr))
);


should be able to store any address family in a dynamically-sized
array field (addr).

I haven't quite been able to figure out how to handle the 
TP_printk() part of this equation. `trace-cmd report` displays
something like "addr=ARG TYPE NOT FIELD BUT 7". 

Thoughts or advice appreciated.

---

Chuck Lever (2):
      SUNRPC: Fix sockaddr handling in the svc_xprt_create_error trace point
      SUNRPC: Fix sockaddr handling in svcsock_accept_class trace points


 include/trace/events/sunrpc.h | 13 +++++++------
 net/sunrpc/svc_xprt.c         |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

--
Chuck Lever

Comments

Steven Rostedt Jan. 10, 2022, 4:05 p.m. UTC | #1
On Mon, 10 Jan 2022 10:56:49 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> The patches in this series address a simple buffer over-read bug in
> the Linux NFS server.
> 
> However I was thinking it would be nice to have trace helpers to
> deal safely with generic socket addresses. I'd like to be able to
> treat them the same way we currently treat strings. So for example:
> 
> 
> #define field_sockaddr(field, len)  __dynamic_array(u8, field, len)
> #define assign_sockaddr(dest, src, len)  memcpy(__get_dynamic_array(dest), src, len)
> #define __get_sockaddr(field)  ((struct sockaddr *)__get_dynamic_array(field))
> 
> TRACE_EVENT(sockaddr_example,
>         TP_PROTO(
>                 const struct sockaddr *sap,
>                 size_t salen
>         ),  
>         TP_ARGS(sap, salen),
>         TP_STRUCT__entry(
>                 __field_sockaddr(addr, salen)
>         ),  
>         TP_fast_assign(
>                 __assign_sockaddr(addr, sap, salen);
>         ),  
>         TP_printk("addr=%pIS", __get_sockaddr(addr))
> );
> 
> 
> should be able to store any address family in a dynamically-sized
> array field (addr).
> 
> I haven't quite been able to figure out how to handle the 
> TP_printk() part of this equation. `trace-cmd report` displays
> something like "addr=ARG TYPE NOT FIELD BUT 7". 
> 
> Thoughts or advice appreciated.

I'll take a look into it.

Thanks,

-- Steve
Steven Rostedt Jan. 10, 2022, 11:31 p.m. UTC | #2
On Mon, 10 Jan 2022 11:05:35 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > I haven't quite been able to figure out how to handle the 
> > TP_printk() part of this equation. `trace-cmd report` displays
> > something like "addr=ARG TYPE NOT FIELD BUT 7". 
> > 
> > Thoughts or advice appreciated.  
> 
> I'll take a look into it.

If you add this to libtraceevent, it will work:

diff --git a/src/event-parse.c b/src/event-parse.c
index 9bd605d..033529c 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -5127,6 +5127,8 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
 	unsigned char *buf;
 	struct sockaddr_storage *sa;
 	bool reverse = false;
+	unsigned int offset;
+	unsigned int len;
 	int rc = 0;
 	int ret;
 
@@ -5152,27 +5154,42 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
 		return rc;
 	}
 
-	if (arg->type != TEP_PRINT_FIELD) {
-		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
-		return rc;
-	}
+	/* evaluate if the arg has a typecast */
+	while (arg->type == TEP_PRINT_TYPE)
+		arg = arg->typecast.item;
+
+	if (arg->type == TEP_PRINT_FIELD) {
 
-	if (!arg->field.field) {
-		arg->field.field =
-			tep_find_any_field(event, arg->field.name);
 		if (!arg->field.field) {
-			do_warning("%s: field %s not found",
-				   __func__, arg->field.name);
-			return rc;
+			arg->field.field =
+				tep_find_any_field(event, arg->field.name);
+			if (!arg->field.field) {
+				do_warning("%s: field %s not found",
+					   __func__, arg->field.name);
+				return rc;
+			}
 		}
+
+		offset = arg->field.field->offset;
+		len = arg->field.field->size;
+
+	} else if (arg->type == TEP_PRINT_DYNAMIC_ARRAY) {
+
+		dynamic_offset_field(event->tep, arg->dynarray.field, data,
+				     size, &offset, &len);
+
+	} else {
+		trace_seq_printf(s, "ARG NOT FIELD NOR DYNAMIC ARRAY BUT TYPE %d",
+				 arg->type);
+		return rc;
 	}
 
-	sa = (struct sockaddr_storage *) (data + arg->field.field->offset);
+	sa = (struct sockaddr_storage *)(data + offset);
 
 	if (sa->ss_family == AF_INET) {
 		struct sockaddr_in *sa4 = (struct sockaddr_in *) sa;
 
-		if (arg->field.field->size < sizeof(struct sockaddr_in)) {
+		if (len < sizeof(struct sockaddr_in)) {
 			trace_seq_printf(s, "INVALIDIPv4");
 			return rc;
 		}
@@ -5185,7 +5202,7 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
 	} else if (sa->ss_family == AF_INET6) {
 		struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *) sa;
 
-		if (arg->field.field->size < sizeof(struct sockaddr_in6)) {
+		if (len < sizeof(struct sockaddr_in6)) {
 			trace_seq_printf(s, "INVALIDIPv6");
 			return rc;
 		}
Chuck Lever III Jan. 11, 2022, 3:40 p.m. UTC | #3
> On Jan 10, 2022, at 6:31 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Mon, 10 Jan 2022 11:05:35 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>>> I haven't quite been able to figure out how to handle the 
>>> TP_printk() part of this equation. `trace-cmd report` displays
>>> something like "addr=ARG TYPE NOT FIELD BUT 7". 
>>> 
>>> Thoughts or advice appreciated.  
>> 
>> I'll take a look into it.
> 
> If you add this to libtraceevent, it will work:

Thank you Steven! I will set up my current test system with
a locally-built trace-cmd and try this out. I can send a
proper patch that introduces the helper macros in my cover
letter's pseudo-code example today or tomorrow.


> diff --git a/src/event-parse.c b/src/event-parse.c
> index 9bd605d..033529c 100644
> --- a/src/event-parse.c
> +++ b/src/event-parse.c
> @@ -5127,6 +5127,8 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
> 	unsigned char *buf;
> 	struct sockaddr_storage *sa;
> 	bool reverse = false;
> +	unsigned int offset;
> +	unsigned int len;
> 	int rc = 0;
> 	int ret;
> 
> @@ -5152,27 +5154,42 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
> 		return rc;
> 	}
> 
> -	if (arg->type != TEP_PRINT_FIELD) {
> -		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
> -		return rc;
> -	}
> +	/* evaluate if the arg has a typecast */
> +	while (arg->type == TEP_PRINT_TYPE)
> +		arg = arg->typecast.item;
> +
> +	if (arg->type == TEP_PRINT_FIELD) {
> 
> -	if (!arg->field.field) {
> -		arg->field.field =
> -			tep_find_any_field(event, arg->field.name);
> 		if (!arg->field.field) {
> -			do_warning("%s: field %s not found",
> -				   __func__, arg->field.name);
> -			return rc;
> +			arg->field.field =
> +				tep_find_any_field(event, arg->field.name);
> +			if (!arg->field.field) {
> +				do_warning("%s: field %s not found",
> +					   __func__, arg->field.name);
> +				return rc;
> +			}
> 		}
> +
> +		offset = arg->field.field->offset;
> +		len = arg->field.field->size;
> +
> +	} else if (arg->type == TEP_PRINT_DYNAMIC_ARRAY) {
> +
> +		dynamic_offset_field(event->tep, arg->dynarray.field, data,
> +				     size, &offset, &len);
> +
> +	} else {
> +		trace_seq_printf(s, "ARG NOT FIELD NOR DYNAMIC ARRAY BUT TYPE %d",
> +				 arg->type);
> +		return rc;
> 	}
> 
> -	sa = (struct sockaddr_storage *) (data + arg->field.field->offset);
> +	sa = (struct sockaddr_storage *)(data + offset);
> 
> 	if (sa->ss_family == AF_INET) {
> 		struct sockaddr_in *sa4 = (struct sockaddr_in *) sa;
> 
> -		if (arg->field.field->size < sizeof(struct sockaddr_in)) {
> +		if (len < sizeof(struct sockaddr_in)) {
> 			trace_seq_printf(s, "INVALIDIPv4");
> 			return rc;
> 		}
> @@ -5185,7 +5202,7 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
> 	} else if (sa->ss_family == AF_INET6) {
> 		struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *) sa;
> 
> -		if (arg->field.field->size < sizeof(struct sockaddr_in6)) {
> +		if (len < sizeof(struct sockaddr_in6)) {
> 			trace_seq_printf(s, "INVALIDIPv6");
> 			return rc;
> 		}

--
Chuck Lever