[PATCH net-next v1] net/9p/trans_fd: check the return value of parse_opts

[PATCH net-next v1] net/9p/trans_fd: check the return value of parse_opts

[Li Zhong]

parse_opts() could fail when there is error parsing mount options into
p9_fd_opts structure due to allocation failure. In that case opts will
contain invalid data.

Signed-off-by: Li Zhong <floridsleeves@gmail.com>
---
 net/9p/trans_fd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index e758978b44be..11ae64c1a24b 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -1061,7 +1061,9 @@ p9_fd_create(struct p9_client *client, const char *addr, char *args)
 	int err;
 	struct p9_fd_opts opts;
 
-	parse_opts(args, &opts);
+	err = parse_opts(args, &opts);
+	if (err < 0)
+		return err;
 	client->trans_opts.fd.rfd = opts.rfd;
 	client->trans_opts.fd.wfd = opts.wfd;
 
-- 
2.25.1

Re: [PATCH net-next v1] net/9p/trans_fd: check the return value of parse_opts

[asmadeus]

Li Zhong wrote on Wed, Sep 21, 2022 at 02:09:21PM -0700:
> parse_opts() could fail when there is error parsing mount options into
> p9_fd_opts structure due to allocation failure. In that case opts will
> contain invalid data.

In practice opts->rfd/wfd is set to ~0 before the failure modes so they
will contain exactly what we want them to contain: something that'll
fail the check below.

It is however cleared like this so I'll queue this patch in 9p tree when
I have a moment, but I'll clarify the commit message to say this is
NO-OP : please feel free to send a v2 if you want to put your own words
in there; otherwise it'll be something like below:
----
net/9p: clarify trans_fd parse_opt failure handling

This parse_opts will set invalid opts.rfd/wfd in case of failure which
we already check, but it is not clear for readers that parse_opts error
are handled in p9_fd_create: clarify this by explicitely checking the
return value.
----


Also, in practice args != null doesn't seem to be checked before (the
parse_opt() in client.c allows it) so keeping the error message common
might be better?
(allocation failure will print its own messages anyway and doesn't need
checking)

> 
> Signed-off-by: Li Zhong <floridsleeves@gmail.com>
> ---
>  net/9p/trans_fd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index e758978b44be..11ae64c1a24b 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -1061,7 +1061,9 @@ p9_fd_create(struct p9_client *client, const char *addr, char *args)
>  	int err;
>  	struct p9_fd_opts opts;
>  
> -	parse_opts(args, &opts);
> +	err = parse_opts(args, &opts);
> +	if (err < 0)
> +		return err;
>  	client->trans_opts.fd.rfd = opts.rfd;
>  	client->trans_opts.fd.wfd = opts.wfd;
>  

Re: [PATCH net-next v1] net/9p/trans_fd: check the return value of parse_opts

[Li Zhong]

On Wed, Sep 21, 2022 at 2:23 PM <asmadeus@codewreck.org> wrote:
>
> Li Zhong wrote on Wed, Sep 21, 2022 at 02:09:21PM -0700:
> > parse_opts() could fail when there is error parsing mount options into
> > p9_fd_opts structure due to allocation failure. In that case opts will
> > contain invalid data.
>
> In practice opts->rfd/wfd is set to ~0 before the failure modes so they
> will contain exactly what we want them to contain: something that'll
> fail the check below.
>
> It is however cleared like this so I'll queue this patch in 9p tree when
> I have a moment, but I'll clarify the commit message to say this is
> NO-OP : please feel free to send a v2 if you want to put your own words
> in there; otherwise it'll be something like below:
> ----
> net/9p: clarify trans_fd parse_opt failure handling
>
> This parse_opts will set invalid opts.rfd/wfd in case of failure which
> we already check, but it is not clear for readers that parse_opts error
> are handled in p9_fd_create: clarify this by explicitly checking the
> return value.
> ----
>

Thanks for the patient reply! I agree that the check on
opts.rfd/wft against ~0 will prevent error even if it fails
memory allocation. But currently the error log is
'insufficient options', which is kind of misleading and the
error code returned is -ENOPROTOOPT instead of -ENOMEM, which
I guess would be better if we distinguish between them.

>
> Also, in practice args != null doesn't seem to be checked before (the
> parse_opt() in client.c allows it) so keeping the error message common
> might be better?
> (allocation failure will print its own messages anyway and doesn't need
> checking)
>
> >
> > Signed-off-by: Li Zhong <floridsleeves@gmail.com>
> > ---
> >  net/9p/trans_fd.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> > index e758978b44be..11ae64c1a24b 100644
> > --- a/net/9p/trans_fd.c
> > +++ b/net/9p/trans_fd.c
> > @@ -1061,7 +1061,9 @@ p9_fd_create(struct p9_client *client, const char *addr, char *args)
> >       int err;
> >       struct p9_fd_opts opts;
> >
> > -     parse_opts(args, &opts);
> > +     err = parse_opts(args, &opts);
> > +     if (err < 0)
> > +             return err;
> >       client->trans_opts.fd.rfd = opts.rfd;
> >       client->trans_opts.fd.wfd = opts.wfd;
> >

Re: [PATCH net-next v1] net/9p/trans_fd: check the return value of parse_opts

[Christian Schoenebeck]

On Donnerstag, 22. September 2022 00:12:38 CEST Li Zhong wrote:
> On Wed, Sep 21, 2022 at 2:23 PM <asmadeus@codewreck.org> wrote:
> > Li Zhong wrote on Wed, Sep 21, 2022 at 02:09:21PM -0700:
> > > parse_opts() could fail when there is error parsing mount options into
> > > p9_fd_opts structure due to allocation failure. In that case opts will
> > > contain invalid data.
> > 
> > In practice opts->rfd/wfd is set to ~0 before the failure modes so they
> > will contain exactly what we want them to contain: something that'll
> > fail the check below.
> > 
> > It is however cleared like this so I'll queue this patch in 9p tree when
> > I have a moment, but I'll clarify the commit message to say this is
> > NO-OP : please feel free to send a v2 if you want to put your own words
> > in there; otherwise it'll be something like below:
> > ----
> > net/9p: clarify trans_fd parse_opt failure handling
> > 
> > This parse_opts will set invalid opts.rfd/wfd in case of failure which
> > we already check, but it is not clear for readers that parse_opts error
> > are handled in p9_fd_create: clarify this by explicitly checking the
> > return value.
> > ----
> 
> Thanks for the patient reply! I agree that the check on
> opts.rfd/wft against ~0 will prevent error even if it fails
> memory allocation. But currently the error log is
> 'insufficient options', which is kind of misleading and the
> error code returned is -ENOPROTOOPT instead of -ENOMEM, which
> I guess would be better if we distinguish between them.

Avoiding those confusions for users makes sense to me, but then please also 
mention that in the commit log, because it is also useful to know the actual 
motivation of a patch.

Best regards,
Christian Schoenebeck