<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 23, 2016 at 6:12 PM, Jeff Darcy <span dir="ltr"><<a href="mailto:jdarcy@redhat.com" target="_blank">jdarcy@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Jiffin found an interesting problem in posix xlator where we have never been<br>
</span>> using setfsuid/gid ( <a href="http://review.gluster.org/#/c/15545/" rel="noreferrer" target="_blank">http://review.gluster.org/#/c/<wbr>15545/</a> ), what I am<br>
<span class="">> seeing regressions after this is, if the files are created using non-root<br>
> user then the file creation fails because that user doesn't have permissions<br>
> to create the gfid-link. So it seems like the correct way forward for this<br>
> patch is to write wrappers around sys_<syscall> to do setfsuid/gid do the<br>
> actual operation requested and then set it back to old uid/gid and then do<br>
> the internal operations. I am planning to write posix_sys_<syscall>() to do<br>
> the same, may be a macro?<br>
<br>
</span>Kind of an aside, but I'd prefer to see a lot fewer macros in our code.<br>
They're not type-safe, and multi-line macros often mess up line numbers for<br>
debugging or error messages. IMO it's better to use functions whenever<br>
possible, and usually to let the compiler worry about how/when to inline.<br>
<span class=""><br>
> I need inputs from you guys to let me know if I am on the right path and if<br>
> you see any issues with this approach.<br>
<br>
</span>I think there's a bit of an interface problem here. The sys_xxx wrappers<br>
don't have arguments that point to the current frame, so how would they get<br>
the correct uid/gid? We could add arguments to each function, but then<br>
we'd have to modify every call. This includes internal calls which don't<br>
have a frame to pass, so I guess they'd have to pass NULL. Alternatively,<br>
we could create a parallel set of functions with frame pointers. Contrary<br>
to what I just said above, this might be a case where macros make sense:<br>
<br>
int<br>
sys_writev_fp (call_frame_t *frame, int fd, void *buf, size_t len)<br>
{<br>
if (frame) { setfsuid(...) ... }<br>
int ret = writev (fd, buf, len);<br>
if (frame) { setfsuid(...) ... }<br>
return ret;<br>
}<br>
#define sys_writev(fd,buf,len) sys_writev_fp (NULL, fd, buf, len)<br>
<br>
That way existing callers don't have to change, but posix can use the<br>
extended versions to get the right setfsuid behavior.<br>
<br>
</blockquote></div><br></div><div class="gmail_extra">After trying to do these modifications to test things out, I am now under the impression to remove setfsuid/gid altogether and depend on posix-acl for permission checks. It seems too cumbersome as the operations more often than not happen on files inside .glusterfs and non-root users/groups don't have permissions at all to access files in that directory.<br></div><div class="gmail_extra"><br clear="all"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Pranith<br></div></div>
</div></div>