[PATCH net-next] devlink: refactor end checks in devlink_nl_cmd_region_read_dumpit

[PATCH net-next] devlink: refactor end checks in devlink_nl_cmd_region_read_dumpit

[Jakub Kicinski]

Clean up after recent fixes, move address calculations
around and change the variable init, so that we can have
just one start_offset == end_offset check.

Make the check a little stricter to preserve the -EINVAL
error if requested start offset is larger than the region
itself.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/devlink.c                            | 41 ++++++++-----------
 .../drivers/net/netdevsim/devlink.sh          | 15 +++++++
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 20f935fa29f5..7b76e5fffc10 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4215,7 +4215,6 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
 						struct nlattr **attrs,
 						u64 start_offset,
 						u64 end_offset,
-						bool dump,
 						u64 *new_offset)
 {
 	struct devlink_snapshot *snapshot;
@@ -4230,9 +4229,6 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
 	if (!snapshot)
 		return -EINVAL;
 
-	if (end_offset > region->size || dump)
-		end_offset = region->size;
-
 	while (curr_offset < end_offset) {
 		u32 data_size;
 		u8 *data;
@@ -4260,13 +4256,12 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 					     struct netlink_callback *cb)
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
-	u64 ret_offset, start_offset, end_offset = 0;
+	u64 ret_offset, start_offset, end_offset = U64_MAX;
 	struct nlattr **attrs = info->attrs;
 	struct devlink_region *region;
 	struct nlattr *chunks_attr;
 	const char *region_name;
 	struct devlink *devlink;
-	bool dump = true;
 	void *hdr;
 	int err;
 
@@ -4294,8 +4289,21 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto out_unlock;
 	}
 
+	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
+	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
+		if (!start_offset)
+			start_offset =
+				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
+
+		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
+		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
+	}
+
+	if (end_offset > region->size)
+		end_offset = region->size;
+
 	/* return 0 if there is no further data to read */
-	if (start_offset >= region->size) {
+	if (start_offset == end_offset) {
 		err = 0;
 		goto out_unlock;
 	}
@@ -4322,27 +4330,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto nla_put_failure;
 	}
 
-	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
-	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
-		if (!start_offset)
-			start_offset =
-				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
-
-		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
-		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
-		dump = false;
-
-		if (start_offset == end_offset) {
-			err = 0;
-			goto nla_put_failure;
-		}
-	}
-
 	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
 						   region, attrs,
 						   start_offset,
-						   end_offset, dump,
-						   &ret_offset);
+						   end_offset, &ret_offset);
 
 	if (err && err != -EMSGSIZE)
 		goto nla_put_failure;
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index ad539eccddcb..de4b32fc4223 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -146,6 +146,21 @@ regions_test()
 
 	check_region_snapshot_count dummy post-first-request 3
 
+	devlink region dump $DL_HANDLE/dummy snapshot 25 >> /dev/null
+	check_err $? "Failed to dump snapshot with id 25"
+
+	devlink region read $DL_HANDLE/dummy snapshot 25 addr 0 len 1 >> /dev/null
+	check_err $? "Failed to read snapshot with id 25 (1 byte)"
+
+	devlink region read $DL_HANDLE/dummy snapshot 25 addr 128 len 128 >> /dev/null
+	check_err $? "Failed to read snapshot with id 25 (128 bytes)"
+
+	devlink region read $DL_HANDLE/dummy snapshot 25 addr 128 len $((1<<32)) >> /dev/null
+	check_err $? "Failed to read snapshot with id 25 (oversized)"
+
+	devlink region read $DL_HANDLE/dummy snapshot 25 addr $((1<<32)) len 128 >> /dev/null 2>&1
+	check_fail $? "Bad read of snapshot with id 25 did not fail"
+
 	devlink region del $DL_HANDLE/dummy snapshot 25
 	check_err $? "Failed to delete snapshot with id 25"
 
-- 
2.25.4

Re: [PATCH net-next] devlink: refactor end checks in devlink_nl_cmd_region_read_dumpit

[Jacob Keller]

On 5/13/2020 10:28 AM, Jakub Kicinski wrote:
> Clean up after recent fixes, move address calculations
> around and change the variable init, so that we can have
> just one start_offset == end_offset check.
> 
> Make the check a little stricter to preserve the -EINVAL
> error if requested start offset is larger than the region
> itself.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/devlink.c                            | 41 ++++++++-----------
>  .../drivers/net/netdevsim/devlink.sh          | 15 +++++++
>  2 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 20f935fa29f5..7b76e5fffc10 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4215,7 +4215,6 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
>  						struct nlattr **attrs,
>  						u64 start_offset,
>  						u64 end_offset,
> -						bool dump,
>  						u64 *new_offset)
>  {
>  	struct devlink_snapshot *snapshot;
> @@ -4230,9 +4229,6 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
>  	if (!snapshot)
>  		return -EINVAL;
>  
> -	if (end_offset > region->size || dump)
> -		end_offset = region->size;
> -

Yea I saw this back when I was looking at enabling region dump without a
snapshot. At this point, it doesn't seem necessary, because the snapshot
time is relatively low, and the changes to make snapshot id a bit easier
to use in scripts (i.e. dynamic generation and saving) mean that it
isn't that useful.

Good to see this cleaned up!

>  	while (curr_offset < end_offset) {
>  		u32 data_size;
>  		u8 *data;
> @@ -4260,13 +4256,12 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  					     struct netlink_callback *cb)
>  {
>  	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> -	u64 ret_offset, start_offset, end_offset = 0;
> +	u64 ret_offset, start_offset, end_offset = U64_MAX;
>  	struct nlattr **attrs = info->attrs;
>  	struct devlink_region *region;
>  	struct nlattr *chunks_attr;
>  	const char *region_name;
>  	struct devlink *devlink;
> -	bool dump = true;
>  	void *hdr;
>  	int err;
>  
> @@ -4294,8 +4289,21 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  		goto out_unlock;
>  	}
>  
> +	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> +	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> +		if (!start_offset)
> +			start_offset =
> +				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> +
> +		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);

At  first, reading this confused me a bit, but it makes sense. The end
is always "beginning + length".

If the start_offset is set before, this simply means that we needed to
read over multiple buffers. Ok.

> +		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
> +	}
> +
> +	if (end_offset > region->size)
> +		end_offset = region->size;
> +
>  	/* return 0 if there is no further data to read */
> -	if (start_offset >= region->size) {
> +	if (start_offset == end_offset) {

Why change this to ==? isn't it possible to specify a start_offset that
is out of bounds? I think this should still be >=

>  		err = 0;
>  		goto out_unlock;
>  	}
> @@ -4322,27 +4330,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  		goto nla_put_failure;
>  	}
>  
> -	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> -	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> -		if (!start_offset)
> -			start_offset =
> -				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> -
> -		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> -		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
> -		dump = false;
> -
> -		if (start_offset == end_offset) {
> -			err = 0;
> -			goto nla_put_failure;
> -		}
> -	}
> -
>  	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
>  						   region, attrs,
>  						   start_offset,
> -						   end_offset, dump,
> -						   &ret_offset);
> +						   end_offset, &ret_offset);
>  
>  	if (err && err != -EMSGSIZE)
>  		goto nla_put_failure;
> diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
> index ad539eccddcb..de4b32fc4223 100755
> --- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
> +++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
> @@ -146,6 +146,21 @@ regions_test()
>  
>  	check_region_snapshot_count dummy post-first-request 3
>  
> +	devlink region dump $DL_HANDLE/dummy snapshot 25 >> /dev/null
> +	check_err $? "Failed to dump snapshot with id 25"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr 0 len 1 >> /dev/null
> +	check_err $? "Failed to read snapshot with id 25 (1 byte)"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr 128 len 128 >> /dev/null
> +	check_err $? "Failed to read snapshot with id 25 (128 bytes)"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr 128 len $((1<<32)) >> /dev/null
> +	check_err $? "Failed to read snapshot with id 25 (oversized)"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr $((1<<32)) len 128 >> /dev/null 2>&1
> +	check_fail $? "Bad read of snapshot with id 25 did not fail"
> +
>  	devlink region del $DL_HANDLE/dummy snapshot 25
>  	check_err $? "Failed to delete snapshot with id 25"
>  
> 

Re: [PATCH net-next] devlink: refactor end checks in devlink_nl_cmd_region_read_dumpit

[Jakub Kicinski]

On Wed, 13 May 2020 13:56:40 -0700 Jacob Keller wrote:
> > @@ -4294,8 +4289,21 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> >  		goto out_unlock;
> >  	}
> >  
> > +	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> > +	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> > +		if (!start_offset)
> > +			start_offset =
> > +				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> > +
> > +		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);  
> 
> At  first, reading this confused me a bit, but it makes sense. The end
> is always "beginning + length".
> 
> If the start_offset is set before, this simply means that we needed to
> read over multiple buffers. Ok.

Yup, I just moved this from below, didn't seem bad enough to rewrite.

> > +		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
> > +	}
> > +
> > +	if (end_offset > region->size)
> > +		end_offset = region->size;
> > +
> >  	/* return 0 if there is no further data to read */
> > -	if (start_offset >= region->size) {
> > +	if (start_offset == end_offset) {  
> 
> Why change this to ==? isn't it possible to specify a start_offset that
> is out of bounds? I think this should still be >=

See 5 lines above :) I moved the capping of end_offset from
devlink_nl_region_read_snapshot_fill() to here. We can keep
it greater equal, but that reads a little defensive.

> >  		err = 0;
> >  		goto out_unlock;
> >  	}

Re: [PATCH net-next] devlink: refactor end checks in devlink_nl_cmd_region_read_dumpit

[David Miller]

From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 13 May 2020 10:28:22 -0700

> Clean up after recent fixes, move address calculations
> around and change the variable init, so that we can have
> just one start_offset == end_offset check.
> 
> Make the check a little stricter to preserve the -EINVAL
> error if requested start offset is larger than the region
> itself.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Applied, thanks.