| Message ID | 20230609174659.60327-6-sprasad@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] cifs: fix status checks in cifs_tree_connect | expand |
On 6/9/2023 1:46 PM, Shyam Prasad N wrote: > iface_cmp used to simply do a memcmp of the two > provided struct sockaddrs. The comparison needs to do more > based on the address family. Similar logic was already > present in cifs_match_ipaddr. Doing something similar now. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > --- > fs/smb/client/cifsglob.h | 37 ----------------------------- > fs/smb/client/cifsproto.h | 1 + > fs/smb/client/connect.c | 50 +++++++++++++++++++++++++++++++++++++++ > fs/smb/client/smb2ops.c | 37 +++++++++++++++++++++++++++++ > 4 files changed, 88 insertions(+), 37 deletions(-) > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h > index 0d84bb1a8cd9..b212a4e16b39 100644 > --- a/fs/smb/client/cifsglob.h > +++ b/fs/smb/client/cifsglob.h > @@ -970,43 +970,6 @@ release_iface(struct kref *ref) > kfree(iface); > } > > -/* > - * compare two interfaces a and b > - * return 0 if everything matches. > - * return 1 if a has higher link speed, or rdma capable, or rss capable > - * return -1 otherwise. > - */ > -static inline int > -iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) > -{ > - int cmp_ret = 0; > - > - WARN_ON(!a || !b); > - if (a->speed == b->speed) { > - if (a->rdma_capable == b->rdma_capable) { > - if (a->rss_capable == b->rss_capable) { > - cmp_ret = memcmp(&a->sockaddr, &b->sockaddr, > - sizeof(a->sockaddr)); > - if (!cmp_ret) > - return 0; > - else if (cmp_ret > 0) > - return 1; > - else > - return -1; > - } else if (a->rss_capable > b->rss_capable) > - return 1; > - else > - return -1; > - } else if (a->rdma_capable > b->rdma_capable) > - return 1; > - else > - return -1; > - } else if (a->speed > b->speed) > - return 1; > - else > - return -1; > -} > - > struct cifs_chan { > unsigned int in_reconnect : 1; /* if session setup in progress for this channel */ > struct TCP_Server_Info *server; > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h > index c1c704990b98..d127aded2f28 100644 > --- a/fs/smb/client/cifsproto.h > +++ b/fs/smb/client/cifsproto.h > @@ -87,6 +87,7 @@ extern int cifs_handle_standard(struct TCP_Server_Info *server, > struct mid_q_entry *mid); > extern int smb3_parse_devname(const char *devname, struct smb3_fs_context *ctx); > extern int smb3_parse_opt(const char *options, const char *key, char **val); > +extern int cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs); > extern bool cifs_match_ipaddr(struct sockaddr *srcaddr, struct sockaddr *rhs); > extern int cifs_discard_remaining_data(struct TCP_Server_Info *server); > extern int cifs_call_async(struct TCP_Server_Info *server, > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c > index 1250d156619b..9d16626e7a66 100644 > --- a/fs/smb/client/connect.c > +++ b/fs/smb/client/connect.c > @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p) > module_put_and_kthread_exit(0); > } > > +int > +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs) Please, please, please, let's not add a new shared entry starting with this four-letter word. > +{ > + struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr; > + struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs; > + struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr; > + struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs; I don't see the value in these variables. Why not simply add the casts in the very few places they're used? > + > + switch (srcaddr->sa_family) { > + case AF_UNSPEC: > + switch (rhs->sa_family) { > + case AF_UNSPEC: > + return 0; > + case AF_INET: > + case AF_INET6: > + return 1; > + default: > + return -1; > + } > + case AF_INET: { > + switch (rhs->sa_family) { > + case AF_UNSPEC: > + return -1; > + case AF_INET: > + return memcmp(saddr4, vaddr4, > + sizeof(struct sockaddr_in)); > + case AF_INET6: > + return 1; > + default: > + return -1; > + } > + } > + case AF_INET6: { > + switch (rhs->sa_family) { > + case AF_UNSPEC: > + case AF_INET: > + return -1; > + case AF_INET6: > + return memcmp(saddr6, > + vaddr6, > + sizeof(struct sockaddr_in6)); > + default: > + return -1; > + } > + } > + default: > + return -1; /* don't expect to be here */ > + } > +} > + > /* > * Returns true if srcaddr isn't specified and rhs isn't specified, or > * if srcaddr is specified and matches the IP address of the rhs argument > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index 18faf267c54d..046341115add 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -513,6 +513,43 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb3_fs_context *ctx) > return rsize; > } > > +/* > + * compare two interfaces a and b > + * return 0 if everything matches. > + * return 1 if a is rdma capable, or rss capable, or has higher link speed > + * return -1 otherwise. > + */ > +static int > +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) > +{ > + int cmp_ret = 0; > + > + WARN_ON(!a || !b); > + if (a->rdma_capable == b->rdma_capable) { > + if (a->rss_capable == b->rss_capable) { > + if (a->speed == b->speed) { > + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr, > + (struct sockaddr *) &b->sockaddr); > + if (!cmp_ret) > + return 0; > + else if (cmp_ret > 0) > + return 1; > + else > + return -1; > + } else if (a->speed > b->speed) > + return 1; > + else > + return -1; > + } else if (a->rss_capable > b->rss_capable) > + return 1; > + else > + return -1; > + } else if (a->rdma_capable > b->rdma_capable) > + return 1; > + else > + return -1; > +} > + The { <0 / 0 / >0 } behavior of this code has been a source of incorrect comparisons in the past, and it still makes my head hurt to attempt a review. So I'll ask, have you thoroughly tested this to be certain that it doesn't result in new channels being created needlessly? > static int > parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, > size_t buf_len, struct cifs_ses *ses, bool in_mount) Tom.
On Fri, Jun 23, 2023 at 12:09:12PM -0400, Tom Talpey wrote: > On 6/9/2023 1:46 PM, Shyam Prasad N wrote: > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c > > index 1250d156619b..9d16626e7a66 100644 > > --- a/fs/smb/client/connect.c > > +++ b/fs/smb/client/connect.c > > @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p) > > module_put_and_kthread_exit(0); > > } > > +int > > +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs) > > Please, please, please, let's not add a new shared entry starting with > this four-letter word. > What would you suggest instead? > > +/* > > + * compare two interfaces a and b > > + * return 0 if everything matches. > > + * return 1 if a is rdma capable, or rss capable, or has higher link speed > > + * return -1 otherwise. > > + */ > > +static int > > +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) > > +{ > > + int cmp_ret = 0; > > + > > + WARN_ON(!a || !b); > > + if (a->rdma_capable == b->rdma_capable) { > > + if (a->rss_capable == b->rss_capable) { > > + if (a->speed == b->speed) { > > + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr, > > + (struct sockaddr *) &b->sockaddr); > > + if (!cmp_ret) > > + return 0; > > + else if (cmp_ret > 0) > > + return 1; > > + else > > + return -1; > > + } else if (a->speed > b->speed) > > + return 1; > > + else > > + return -1; > > + } else if (a->rss_capable > b->rss_capable) > > + return 1; > > + else > > + return -1; > > + } else if (a->rdma_capable > b->rdma_capable) > > + return 1; > > + else > > + return -1; > > +} > > + > > The { <0 / 0 / >0 } behavior of this code has been a source of > incorrect comparisons in the past, and it still makes my head hurt > to attempt a review. > > So I'll ask, have you thoroughly tested this to be certain that it > doesn't result in new channels being created needlessly? I was not a huge fan of this function and the move makes it harder to review. But I didn't see anything wrong with it.... Here is a slightly simplified diff that I use to review. regards, dan carpenter iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) { int cmp_ret = 0; WARN_ON(!a || !b); - if (a->speed == b->speed) { if (a->rdma_capable == b->rdma_capable) { if (a->rss_capable == b->rss_capable) { - cmp_ret = memcmp(&a->sockaddr, &b->sockaddr, - sizeof(a->sockaddr)); + if (a->speed == b->speed) { + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr, + (struct sockaddr *) &b->sockaddr); if (!cmp_ret) return 0; else if (cmp_ret > 0) return 1; else return -1; - } else if (a->rss_capable > b->rss_capable) + } else if (a->speed > b->speed) return 1; else return -1; - } else if (a->rdma_capable > b->rdma_capable) + } else if (a->rss_capable > b->rss_capable) return 1; else return -1; - } else if (a->speed > b->speed) + } else if (a->rdma_capable > b->rdma_capable) return 1; else return -1; } regards, dan carpenter
On 6/26/2023 7:12 AM, Dan Carpenter wrote: > On Fri, Jun 23, 2023 at 12:09:12PM -0400, Tom Talpey wrote: >> On 6/9/2023 1:46 PM, Shyam Prasad N wrote: >>> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c >>> index 1250d156619b..9d16626e7a66 100644 >>> --- a/fs/smb/client/connect.c >>> +++ b/fs/smb/client/connect.c >>> @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p) >>> module_put_and_kthread_exit(0); >>> } >>> +int >>> +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs) >> >> Please, please, please, let's not add a new shared entry starting with >> this four-letter word. >> > > What would you suggest instead? "smb" would be fine, but honestly it's only referenced in one place and used to be static inline, so it kind of doesn't matter what it's called. It's also unclear to me why it's being moved to a .c file and made visible. Anything but "cifs" though. Tom. > >>> +/* >>> + * compare two interfaces a and b >>> + * return 0 if everything matches. >>> + * return 1 if a is rdma capable, or rss capable, or has higher link speed >>> + * return -1 otherwise. >>> + */ >>> +static int >>> +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) >>> +{ >>> + int cmp_ret = 0; >>> + >>> + WARN_ON(!a || !b); >>> + if (a->rdma_capable == b->rdma_capable) { >>> + if (a->rss_capable == b->rss_capable) { >>> + if (a->speed == b->speed) { >>> + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr, >>> + (struct sockaddr *) &b->sockaddr); >>> + if (!cmp_ret) >>> + return 0; >>> + else if (cmp_ret > 0) >>> + return 1; >>> + else >>> + return -1; >>> + } else if (a->speed > b->speed) >>> + return 1; >>> + else >>> + return -1; >>> + } else if (a->rss_capable > b->rss_capable) >>> + return 1; >>> + else >>> + return -1; >>> + } else if (a->rdma_capable > b->rdma_capable) >>> + return 1; >>> + else >>> + return -1; >>> +} >>> + >> >> The { <0 / 0 / >0 } behavior of this code has been a source of >> incorrect comparisons in the past, and it still makes my head hurt >> to attempt a review. >> >> So I'll ask, have you thoroughly tested this to be certain that it >> doesn't result in new channels being created needlessly? > > I was not a huge fan of this function and the move makes it harder to > review. But I didn't see anything wrong with it.... Here is a slightly > simplified diff that I use to review. > > regards, > dan carpenter > > iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) > { > int cmp_ret = 0; > > WARN_ON(!a || !b); > - if (a->speed == b->speed) { > if (a->rdma_capable == b->rdma_capable) { > if (a->rss_capable == b->rss_capable) { > - cmp_ret = memcmp(&a->sockaddr, &b->sockaddr, > - sizeof(a->sockaddr)); > + if (a->speed == b->speed) { > + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr, > + (struct sockaddr *) &b->sockaddr); > if (!cmp_ret) > return 0; > else if (cmp_ret > 0) > return 1; > else > return -1; > - } else if (a->rss_capable > b->rss_capable) > + } else if (a->speed > b->speed) > return 1; > else > return -1; > - } else if (a->rdma_capable > b->rdma_capable) > + } else if (a->rss_capable > b->rss_capable) > return 1; > else > return -1; > - } else if (a->speed > b->speed) > + } else if (a->rdma_capable > b->rdma_capable) > return 1; > else > return -1; > } > > regards, > dan carpenter > >
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h index 0d84bb1a8cd9..b212a4e16b39 100644 --- a/fs/smb/client/cifsglob.h +++ b/fs/smb/client/cifsglob.h @@ -970,43 +970,6 @@ release_iface(struct kref *ref) kfree(iface); } -/* - * compare two interfaces a and b - * return 0 if everything matches. - * return 1 if a has higher link speed, or rdma capable, or rss capable - * return -1 otherwise. - */ -static inline int -iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) -{ - int cmp_ret = 0; - - WARN_ON(!a || !b); - if (a->speed == b->speed) { - if (a->rdma_capable == b->rdma_capable) { - if (a->rss_capable == b->rss_capable) { - cmp_ret = memcmp(&a->sockaddr, &b->sockaddr, - sizeof(a->sockaddr)); - if (!cmp_ret) - return 0; - else if (cmp_ret > 0) - return 1; - else - return -1; - } else if (a->rss_capable > b->rss_capable) - return 1; - else - return -1; - } else if (a->rdma_capable > b->rdma_capable) - return 1; - else - return -1; - } else if (a->speed > b->speed) - return 1; - else - return -1; -} - struct cifs_chan { unsigned int in_reconnect : 1; /* if session setup in progress for this channel */ struct TCP_Server_Info *server; diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h index c1c704990b98..d127aded2f28 100644 --- a/fs/smb/client/cifsproto.h +++ b/fs/smb/client/cifsproto.h @@ -87,6 +87,7 @@ extern int cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid); extern int smb3_parse_devname(const char *devname, struct smb3_fs_context *ctx); extern int smb3_parse_opt(const char *options, const char *key, char **val); +extern int cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs); extern bool cifs_match_ipaddr(struct sockaddr *srcaddr, struct sockaddr *rhs); extern int cifs_discard_remaining_data(struct TCP_Server_Info *server); extern int cifs_call_async(struct TCP_Server_Info *server, diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c index 1250d156619b..9d16626e7a66 100644 --- a/fs/smb/client/connect.c +++ b/fs/smb/client/connect.c @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p) module_put_and_kthread_exit(0); } +int +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs) +{ + struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr; + struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs; + struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr; + struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs; + + switch (srcaddr->sa_family) { + case AF_UNSPEC: + switch (rhs->sa_family) { + case AF_UNSPEC: + return 0; + case AF_INET: + case AF_INET6: + return 1; + default: + return -1; + } + case AF_INET: { + switch (rhs->sa_family) { + case AF_UNSPEC: + return -1; + case AF_INET: + return memcmp(saddr4, vaddr4, + sizeof(struct sockaddr_in)); + case AF_INET6: + return 1; + default: + return -1; + } + } + case AF_INET6: { + switch (rhs->sa_family) { + case AF_UNSPEC: + case AF_INET: + return -1; + case AF_INET6: + return memcmp(saddr6, + vaddr6, + sizeof(struct sockaddr_in6)); + default: + return -1; + } + } + default: + return -1; /* don't expect to be here */ + } +} + /* * Returns true if srcaddr isn't specified and rhs isn't specified, or * if srcaddr is specified and matches the IP address of the rhs argument diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 18faf267c54d..046341115add 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -513,6 +513,43 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb3_fs_context *ctx) return rsize; } +/* + * compare two interfaces a and b + * return 0 if everything matches. + * return 1 if a is rdma capable, or rss capable, or has higher link speed + * return -1 otherwise. + */ +static int +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b) +{ + int cmp_ret = 0; + + WARN_ON(!a || !b); + if (a->rdma_capable == b->rdma_capable) { + if (a->rss_capable == b->rss_capable) { + if (a->speed == b->speed) { + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr, + (struct sockaddr *) &b->sockaddr); + if (!cmp_ret) + return 0; + else if (cmp_ret > 0) + return 1; + else + return -1; + } else if (a->speed > b->speed) + return 1; + else + return -1; + } else if (a->rss_capable > b->rss_capable) + return 1; + else + return -1; + } else if (a->rdma_capable > b->rdma_capable) + return 1; + else + return -1; +} + static int parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, size_t buf_len, struct cifs_ses *ses, bool in_mount)
iface_cmp used to simply do a memcmp of the two provided struct sockaddrs. The comparison needs to do more based on the address family. Similar logic was already present in cifs_match_ipaddr. Doing something similar now. Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> --- fs/smb/client/cifsglob.h | 37 ----------------------------- fs/smb/client/cifsproto.h | 1 + fs/smb/client/connect.c | 50 +++++++++++++++++++++++++++++++++++++++ fs/smb/client/smb2ops.c | 37 +++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 37 deletions(-)