Re: [REVIEW][PATCH 1/3] vfs: In d_path don't call d_dname on a mount point

Re: [REVIEW][PATCH 1/3] vfs: In d_path don't call d_dname on a mount point

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 1647 bytes --]

On Sat, 30 Nov 2013 17:02:26 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:


> BTW, what happens if svc_export_request() ends up with pathname filling
> almost all space left, so that qword_add(bpp, blen, pth) right after the
> call of d_path() in there overwrites the beginning of d_path() output?
> Neil?  And while we are at it, handling of overflow in there looks also
> looks fishy...

In this case the returned *blen will be negative so cache_request() in
net/sunrpc/cache.h will return -EAGAIN.
cache_read() would then go into a tight loop, trying again and again to
create the request, but it will never fit in the buffer.

I guess maybe an EINVAL might help there, plus code to skip over impossible
requests.
Maybe the following.  We'd need to double check that no ->cache_request
function can fail in a way that is worth retrying, but I doubt they would.

Any thoughts Bruce?

Thanks,
NeilBrown


diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index a72de074172d..a065f827e2a3 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -748,7 +748,7 @@ static int cache_request(struct cache_detail *detail,
 
 	detail->cache_request(detail, crq->item, &bp, &len);
 	if (len < 0)
-		return -EAGAIN;
+		return -EINVAL;
 	return PAGE_SIZE - len;
 }
 
@@ -788,6 +788,11 @@ static ssize_t cache_read(struct file *filp, char __user *buf, size_t count,
 
 	if (rq->len == 0) {
 		err = cache_request(cd, rq);
+		if (err == -EINVAL) {
+			spin_lock(&queue_lock);
+			list_move(&rp->q.list, &rq->q.list);
+			spin_unlock(&queue_lock);
+		}
 		if (err < 0)
 			goto out;
 		rq->len = err;

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 171 bytes --]

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [REVIEW][PATCH 1/3] vfs: In d_path don't call d_dname on a mount point

[J.Bruce Fields]

On Mon, Dec 02, 2013 at 04:43:59PM +1100, NeilBrown wrote:
> On Sat, 30 Nov 2013 17:02:26 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> 
> > BTW, what happens if svc_export_request() ends up with pathname filling
> > almost all space left, so that qword_add(bpp, blen, pth) right after the
> > call of d_path() in there overwrites the beginning of d_path() output?
> > Neil?  And while we are at it, handling of overflow in there looks also
> > looks fishy...
> 
> In this case the returned *blen will be negative so cache_request() in
> net/sunrpc/cache.h will return -EAGAIN.
> cache_read() would then go into a tight loop, trying again and again to
> create the request, but it will never fit in the buffer.
> 
> I guess maybe an EINVAL might help there, plus code to skip over impossible
> requests.
> Maybe the following.  We'd need to double check that no ->cache_request
> function can fail in a way that is worth retrying, but I doubt they would.
> 
> Any thoughts Bruce?

Yes, the cache_request failures are all similarly fatal overflows.

Uh, not sure if I remember how the data structures here work: so
userspace is now going to get an -EINVAL on read, and may just end up
retrying the read?

Or do we hit the "need to release rq" case in cache_read and just
discard the request?

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index a72de074172d..a065f827e2a3 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -748,7 +748,7 @@ static int cache_request(struct cache_detail *detail,
>  
>  	detail->cache_request(detail, crq->item, &bp, &len);
>  	if (len < 0)
> -		return -EAGAIN;
> +		return -EINVAL;
>  	return PAGE_SIZE - len;
>  }
>  
> @@ -788,6 +788,11 @@ static ssize_t cache_read(struct file *filp, char __user *buf, size_t count,
>  
>  	if (rq->len == 0) {
>  		err = cache_request(cd, rq);
> +		if (err == -EINVAL) {
> +			spin_lock(&queue_lock);
> +			list_move(&rp->q.list, &rq->q.list);
> +			spin_unlock(&queue_lock);
> +		}
>  		if (err < 0)
>  			goto out;
>  		rq->len = err;