[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Jan Kryl wrote: > > Sounds reasonable. Webrev updated: > > http://cr.opensolaris.org/~jkryl/nfs-utf8-3/ > > thanks > Jan > Looks good
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
On 03/02/10 16:11 -0600, Tom Haynes wrote: > usr/src/uts/common/fs/nfs/nfs4_srv.c: > > 1178 } > 1179 /* If necessary, convert to UTF-8 for ill behaved clients */ > 1180 1181 ca = (struct sockaddr > *)svc_getrpccaller(req->rq_xprt)->buf; > > > I'd prefer a line between 1178 and 1179. And take away 1180. > > The first point is more of a concern. > > > > Actually, all of the instances of that comment could probably go. It is > copied everywhere that nfscmd_convname() appears. It would be better served > as a comment to the function. > done. > == > > usr/src/uts/common/fs/nfs/nfs_server.c > > > Line 3219 does not need to be added. > typo fixed thanks Jan
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
On 03/02/10 16:20 -0600, Tom Haynes wrote: > Jan Kryl wrote: > > Hi Rob, > > > > On 03/02/10 11:53 -0700, Robert Thurlow wrote: > > > >> Jan Kryl wrote: > >> > >>> Hi, > >>> please could you review the following fix for 6920999? > >>> webrev: > >>> http://cr.opensolaris.org/~jkryl/nfs-utf8/ > >>> > >> Hi Jan, > >> > >> The changes look good to me except for something related > >> to the ASSERT change Tom mentioned. I think ASSERTs and > >> a runtime check should be put in nfscmd_findmap(), which > >> is the highest point that is vulnerable to a stray NULL > >> pointer. > >> > >> > > I have removed asserts and added a NULL-test in > > nfscmd_findmap() as you have suggested. I think that > > the asserts aren't usefull anymore now when we test > > NULL values for both debug and non-debug kernels. > > > > > > The asserts are still useful in that they let us know about > a case that should never happen. In a debug kernel, we > want to know about this condition. I.e., it probably > will be hit by a developer during unit testing when > they introduce a bug in this area. > > In a non-debug kernel, for this case, we happen to be > able to recover in a fashion that can be handled by > the caller. In most cases like this, a panic occurs > and we get a customer support call. > > So please add the asserts back in - we'd like some > observability to the issue. > Sounds reasonable. Webrev updated: http://cr.opensolaris.org/~jkryl/nfs-utf8-3/ thanks Jan
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Hi Rob, On 03/02/10 11:53 -0700, Robert Thurlow wrote: > Jan Kryl wrote: > > Hi, > > please could you review the following fix for 6920999? > > webrev: > > http://cr.opensolaris.org/~jkryl/nfs-utf8/ > > Hi Jan, > > The changes look good to me except for something related > to the ASSERT change Tom mentioned. I think ASSERTs and > a runtime check should be put in nfscmd_findmap(), which > is the highest point that is vulnerable to a stray NULL > pointer. > I have removed asserts and added a NULL-test in nfscmd_findmap() as you have suggested. I think that the asserts aren't usefull anymore now when we test NULL values for both debug and non-debug kernels. > Thanks for running with this, I had not appreciated there > was incomplete implementation on the server as well. The > CR should be clear that this problem and this fix is about > the server side, not the client. > I have added a clear statement to evaluation of CR to clarify this. Please see new revision of webrev: http://cr.opensolaris.org/~jkryl/nfs-utf8-3/ Thanks for your comments -Jan
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Hi Tom,
On 03/02/10 11:16 -0600, Tom Haynes wrote:
> usr/src/uts/common/fs/nfs/nfs_cmd.c:
>
> 197 * First try to find a cached entry. If not successfull,
>
>
> One 'l' as successful
>
fixed
>
>
> 224 if (charset == NULL)
> 225 /* the slooow way - ask daemon */
> 226 charset = nfscmd_charmap(exi, sp);
>
> I'd prefer 225 before 224.
>
done
> -
>
> 276 ASSERT(exi != NULL);
> 277 ASSERT(sp != NULL);
>
> Handles debug. For production, why not add the following as well:
>
> if (exi == NULL || sp == NULL)
> return (NULL);
>
> Actually, this is a major requirement. nfscmd_charmap() used to
> handle this without a potential panic. Now it will panic.
>
Asserts substituted by if condition and moved to nfscmd_findmap()
as suggested by Rob.
>
> ---
>
>
> usr/src/uts/common/fs/nfs/nfs4_srv.c
>
>
>
>
> 6067 static nfsstat4
> 6068 rfs4_lookup(component4 *component, struct svc_req *req,
> 6069 struct compound_state *cs)
>
> 6097 /* If necessary, convert to UTF-8 for illbehaved clients */
> 6098 6099 ca = (struct sockaddr
> *)svc_getrpccaller(req->rq_xprt)->buf;
> 6100 name = nfscmd_convname(ca, cs->exi, nm, NFSCMD_CONV_INBOUND,
> 6101 MAXPATHLEN + 1);
> 6102 6103 if (name == NULL) {
>
> So now every lookup will make a call to userland regardless.
>
> What is the performance impact?
>
> Is there a quick check that can be done here to see if we need to make
> the call?
>
> Is there a way to cache results or the fact that a cs->exi is known
> to be well behaved?
>
Already answered by Rob.
> ---
>
> BTW: it should be "ill behaved" on 6097
>
fixed.
Thanks for your comments! Please, see new revision of webrev:
http://cr.opensolaris.org/~jkryl/nfs-utf8-3/
-jan
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Hi, please could you review the following fix for 6920999? webrev: http://cr.opensolaris.org/~jkryl/nfs-utf8/ thanks Jan Background information (copy-pasted from bugster): -- When fixing this I have encountered a couple of independent bugs, which all disabled proper function of utf-8 to other character set and vice versa conversion. This CR fixes all of these defects. 1) definition of in_access_list() in mountd.c was modified (parameter count changed) but this change was not propagated to nfs_cmd.c, which also used this function. This led to core dump of mountd daemon upon receiving NFS_CMD request. This is regression of 6882460 integrated in snv 130. 2) rfs4_lookup() in nfsv4 server code, didn't attempt to translate between character sets. This could be easily reproduced by copying file containing utf-8 characters from zfs share to local disk. This has been there probably since this code was written. 3) nfscmd_send() which is responsible for communication with mountd daemon, returned misleading return value when door fs call was successfull, but the request to mountd daemon itself failed. The function should return 0 (NFSCMD_ERR_SUCCESS) only if both conditions are met: door fs call was successfull and mountd request was successfull. 4) nfscmd_charmap() was never called for NFSv4 compound requests. Thus the charset cache didn't get initialized for a client and the need for charset conversion could not be detected. For NFS v3, v2 we called nfscmd_charmap() before the request was processed. This approach is not possible in case of NFS v4, because compound calls can cross filesystem boundaries and we wouldn't know, which exportinfo to associate client with. Thus I have modified all NFS versions to defer call to nfscmd_charmap() to a point where it is actually needed (when we do a translation). At that time we know exactly which exportinfo should be used. Note that all of this is in case of NFS v4 just a workaround for misbehaved NFS clients, which send character sequences which aren't valid utf-8 sequences, which is prohibited by standard. See 6862326 for more details.
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Jan Kryl wrote: > Hi Rob, > > On 03/02/10 11:53 -0700, Robert Thurlow wrote: > >> Jan Kryl wrote: >> >>> Hi, >>> please could you review the following fix for 6920999? >>> webrev: >>> http://cr.opensolaris.org/~jkryl/nfs-utf8/ >>> >> Hi Jan, >> >> The changes look good to me except for something related >> to the ASSERT change Tom mentioned. I think ASSERTs and >> a runtime check should be put in nfscmd_findmap(), which >> is the highest point that is vulnerable to a stray NULL >> pointer. >> >> > I have removed asserts and added a NULL-test in > nfscmd_findmap() as you have suggested. I think that > the asserts aren't usefull anymore now when we test > NULL values for both debug and non-debug kernels. > > The asserts are still useful in that they let us know about a case that should never happen. In a debug kernel, we want to know about this condition. I.e., it probably will be hit by a developer during unit testing when they introduce a bug in this area. In a non-debug kernel, for this case, we happen to be able to recover in a fashion that can be handled by the caller. In most cases like this, a panic occurs and we get a customer support call. So please add the asserts back in - we'd like some observability to the issue.
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
usr/src/uts/common/fs/nfs/nfs4_srv.c: 1178 } 1179 /* If necessary, convert to UTF-8 for ill behaved clients */ 1180 1181 ca = (struct sockaddr *)svc_getrpccaller(req->rq_xprt)->buf; I'd prefer a line between 1178 and 1179. And take away 1180. The first point is more of a concern. Actually, all of the instances of that comment could probably go. It is copied everywhere that nfscmd_convname() appears. It would be better served as a comment to the function. == usr/src/uts/common/fs/nfs/nfs_server.c Line 3219 does not need to be added.
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Jan Kryl wrote: > Hi, > > please could you review the following fix for 6920999? > > webrev: > http://cr.opensolaris.org/~jkryl/nfs-utf8/ Hi Jan, The changes look good to me except for something related to the ASSERT change Tom mentioned. I think ASSERTs and a runtime check should be put in nfscmd_findmap(), which is the highest point that is vulnerable to a stray NULL pointer. Thanks for running with this, I had not appreciated there was incomplete implementation on the server as well. The CR should be clear that this problem and this fix is about the server side, not the client. Rob T
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
Tom Haynes wrote:
> usr/src/uts/common/fs/nfs/nfs4_srv.c
>
> 6067 static nfsstat4
> 6068 rfs4_lookup(component4 *component, struct svc_req *req,
> 6069 struct compound_state *cs)
>
> 6097 /* If necessary, convert to UTF-8 for illbehaved clients */
> 6098 6099 ca = (struct sockaddr
> *)svc_getrpccaller(req->rq_xprt)->buf;
> 6100 name = nfscmd_convname(ca, cs->exi, nm, NFSCMD_CONV_INBOUND,
> 6101 MAXPATHLEN + 1);
> 6102 6103 if (name == NULL) {
>
> So now every lookup will make a call to userland regardless.
Not as I read it - nfscmd_convname() calls nfscmd_findmap(),
which searches an in-kernel list, calling nfscmd_charmap() to
do the upcall only if a map is not found.
Rob T
[nfs-discuss] code review 6920999 (NFSv4 and utf-8)
usr/src/uts/common/fs/nfs/nfs_cmd.c:
197 * First try to find a cached entry. If not successfull,
One 'l' as successful
224 if (charset == NULL)
225 /* the slooow way - ask daemon */
226 charset = nfscmd_charmap(exi, sp);
I'd prefer 225 before 224.
-
276 ASSERT(exi != NULL);
277 ASSERT(sp != NULL);
Handles debug. For production, why not add the following as well:
if (exi == NULL || sp == NULL)
return (NULL);
Actually, this is a major requirement. nfscmd_charmap() used to
handle this without a potential panic. Now it will panic.
---
usr/src/uts/common/fs/nfs/nfs4_srv.c
6067 static nfsstat4
6068 rfs4_lookup(component4 *component, struct svc_req *req,
6069 struct compound_state *cs)
6097 /* If necessary, convert to UTF-8 for illbehaved clients */
6098
6099 ca = (struct sockaddr *)svc_getrpccaller(req->rq_xprt)->buf;
6100 name = nfscmd_convname(ca, cs->exi, nm, NFSCMD_CONV_INBOUND,
6101 MAXPATHLEN + 1);
6102
6103 if (name == NULL) {
So now every lookup will make a call to userland regardless.
What is the performance impact?
Is there a quick check that can be done here to see if we need to make
the call?
Is there a way to cache results or the fact that a cs->exi is known
to be well behaved?
---
BTW: it should be "ill behaved" on 6097
