shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Jeff Layton]

I've been looking at replacing the current scheme that knfsd uses to
track client_id4's (aka the v4recoverydir stuff), with an
upcall/downcall scheme. Primarily this is to allow for more robust
handling of clustered NFSv4 services.

In the process, I've been looking at the various upcall schemes we use
to see which ones might be suitable to use in this effort. I've noticed
that we have several upcalls that use rpc_pipefs, and that all of them
seem to make assumptions that the userspace programs will align their
message structs identically to how the kernel does.

For instance, here's the idmap one:

struct idmap_msg {
        __u8  im_type;
        __u8  im_conv;
        char  im_name[IDMAP_NAMESZ];
        __u32 im_id;
        __u8  im_status;
};

Note that this struct does not have __attribute__((packed)), so the
compiler is allowed to add padding between the fields as it sees fit.

If, for instance, someone were to build the userspace programs
differently than the kernel (for instance x86_64 kernel with i686
userspace), it's possible that the padding between them would be
different. It's also possible that different compilers might align
things differently here.

The blocklayout upcall is even more scary as the width of the status
field is not explicit:

struct bl_dev_msg {
        int status;
        uint32_t major, minor;
};

...it's unlikely that the kernel and userspace would differ on the size
of an int here, but it might be a good idea to go ahead and make that
explicitly 32 bits in case we end up dealing with more exotic arches at
some point in the future.

I'm not sure what we can really do about this at this point. Adding
this attribute now would definitely be an kernel/userspace
compatibility issue.

One possibility is to add padding between the fields that aligns with
the current padding that the compiler adds and then make them "packed".
That might make these structs arch-specific though since different
arches probably pad these differently... :-/

Am I making mountains out of molehills here? Thoughts?

-- 
Jeff Layton <jlayton@redhat.com>

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Jim Rees]

Jeff Layton wrote:

  The blocklayout upcall is even more scary as the width of the status
  field is not explicit:
  
  struct bl_dev_msg {
          int status;
          uint32_t major, minor;
  };

I'll take the blame for that one.  I will queue up a fix.

Making the blocklayout upcall struct packed might still be possible since
it's not officially released until 3.1, but I'm terrified of making changes
at this point in the release cycle that aren't actual bug fixes.

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Jeff Layton]

On Fri, 9 Sep 2011 15:56:15 -0400
Jim Rees <rees@umich.edu> wrote:

> Jeff Layton wrote:
> 
>   The blocklayout upcall is even more scary as the width of the status
>   field is not explicit:
>   
>   struct bl_dev_msg {
>           int status;
>           uint32_t major, minor;
>   };
> 
> I'll take the blame for that one.  I will queue up a fix.
> 
> Making the blocklayout upcall struct packed might still be possible since
> it's not officially released until 3.1, but I'm terrified of making changes
> at this point in the release cycle that aren't actual bug fixes.

Thanks, though I guess I also should take some of the blame for not
reviewing and noticing this earlier...

I'd personally call this a bug, and one that's particularly important
to fix sooner rather than later. Changing this will mean ABI breakage
any way you look at it. I think it would be better to go through that
pain now before anyone is really relying on that code.

While we're looking at this...do we also need to worry about endianness
here? Is it possible we'd ever end up running BE upcall code on a LE
kernel (or vice versa) in some sort of horrid compat mode? If so, it
might be worthwhile to consider making both those fields __be32 or
something and fixing the code to handle that properly as well.

-- 
Jeff Layton <jlayton@redhat.com>

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Jim Rees]

Jeff Layton wrote:

  On Fri, 9 Sep 2011 15:56:15 -0400
  Jim Rees <rees@umich.edu> wrote:
  
  > Jeff Layton wrote:
  > 
  >   The blocklayout upcall is even more scary as the width of the status
  >   field is not explicit:
  >   
  >   struct bl_dev_msg {
  >           int status;
  >           uint32_t major, minor;
  >   };
  > 
  > I'll take the blame for that one.  I will queue up a fix.
  > 
  > Making the blocklayout upcall struct packed might still be possible since
  > it's not officially released until 3.1, but I'm terrified of making changes
  > at this point in the release cycle that aren't actual bug fixes.
  
  Thanks, though I guess I also should take some of the blame for not
  reviewing and noticing this earlier...
  
  I'd personally call this a bug, and one that's particularly important
  to fix sooner rather than later. Changing this will mean ABI breakage
  any way you look at it. I think it would be better to go through that
  pain now before anyone is really relying on that code.

I'll go with whatever Trond thinks is best (not to shirk the responsibility,
but he's better able to assess the risks than I).  Should I send a patch?
It needs to be coordinated with nfs-utils, but that hasn't been released yet
either.

Would packing this struct actually change the layout on either x86 or
x86_64?

  While we're looking at this...do we also need to worry about endianness
  here? Is it possible we'd ever end up running BE upcall code on a LE
  kernel (or vice versa) in some sort of horrid compat mode? If so, it
  might be worthwhile to consider making both those fields __be32 or
  something and fixing the code to handle that properly as well.

If we're going to go to that much trouble, I think I would ditch the binary
and go with text.  The upcall for block layout is not in a performance
critical path.

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Trond Myklebust]

On Fri, 2011-09-09 at 18:03 -0400, Jim Rees wrote: 
> Jeff Layton wrote:
> 
>   On Fri, 9 Sep 2011 15:56:15 -0400
>   Jim Rees <rees@umich.edu> wrote:
>   
>   > Jeff Layton wrote:
>   > 
>   >   The blocklayout upcall is even more scary as the width of the status
>   >   field is not explicit:
>   >   
>   >   struct bl_dev_msg {
>   >           int status;
>   >           uint32_t major, minor;
>   >   };
>   > 
>   > I'll take the blame for that one.  I will queue up a fix.
>   > 
>   > Making the blocklayout upcall struct packed might still be possible since
>   > it's not officially released until 3.1, but I'm terrified of making changes
>   > at this point in the release cycle that aren't actual bug fixes.
>   
>   Thanks, though I guess I also should take some of the blame for not
>   reviewing and noticing this earlier...
>   
>   I'd personally call this a bug, and one that's particularly important
>   to fix sooner rather than later. Changing this will mean ABI breakage
>   any way you look at it. I think it would be better to go through that
>   pain now before anyone is really relying on that code.
> 
> I'll go with whatever Trond thinks is best (not to shirk the responsibility,
> but he's better able to assess the risks than I).  Should I send a patch?
> It needs to be coordinated with nfs-utils, but that hasn't been released yet
> either.

Since the type is an 'int' rather than a 'long', I don't think we're
actually breaking any ABIs. I can't think of any currently supported
Linux platforms where 'int' is anything other than a 32-bit integer.
That said, it is good to be specific whenever nailing down an ABI.

> Would packing this struct actually change the layout on either x86 or
> x86_64?

Possibly, but we don't pack the other upcall/downcall arguments. See, for instance, the RPCSEC_GSS contexts.

> While we're looking at this...do we also need to worry about endianness
>   here? Is it possible we'd ever end up running BE upcall code on a LE
>   kernel (or vice versa) in some sort of horrid compat mode? If so, it
>   might be worthwhile to consider making both those fields __be32 or
>   something and fixing the code to handle that properly as well.
> 
> If we're going to go to that much trouble, I think I would ditch the binary
> and go with text.  The upcall for block layout is not in a performance
> critical path.

Urgh... If we're mixing endian modes, won't system calls be the first
things to break?
In any case, the current NFS client upcalls suppose that the endianness
stays the same between kernel and userland.

Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Jeff Layton]

On Fri, 09 Sep 2011 18:32:01 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2011-09-09 at 18:03 -0400, Jim Rees wrote: 
> > Jeff Layton wrote:
> > 
> >   On Fri, 9 Sep 2011 15:56:15 -0400
> >   Jim Rees <rees@umich.edu> wrote:
> >   
> >   > Jeff Layton wrote:
> >   > 
> >   >   The blocklayout upcall is even more scary as the width of the status
> >   >   field is not explicit:
> >   >   
> >   >   struct bl_dev_msg {
> >   >           int status;
> >   >           uint32_t major, minor;
> >   >   };
> >   > 
> >   > I'll take the blame for that one.  I will queue up a fix.
> >   > 
> >   > Making the blocklayout upcall struct packed might still be possible since
> >   > it's not officially released until 3.1, but I'm terrified of making changes
> >   > at this point in the release cycle that aren't actual bug fixes.
> >   
> >   Thanks, though I guess I also should take some of the blame for not
> >   reviewing and noticing this earlier...
> >   
> >   I'd personally call this a bug, and one that's particularly important
> >   to fix sooner rather than later. Changing this will mean ABI breakage
> >   any way you look at it. I think it would be better to go through that
> >   pain now before anyone is really relying on that code.
> > 
> > I'll go with whatever Trond thinks is best (not to shirk the responsibility,
> > but he's better able to assess the risks than I).  Should I send a patch?
> > It needs to be coordinated with nfs-utils, but that hasn't been released yet
> > either.
> 

Agreed. I would defer to Trond's judgement on this as well...

> Since the type is an 'int' rather than a 'long', I don't think we're
> actually breaking any ABIs. I can't think of any currently supported
> Linux platforms where 'int' is anything other than a 32-bit integer.
> That said, it is good to be specific whenever nailing down an ABI.
> 

The operative term is "currently supported". Will that always be the
case? Quite possibly, but since we have a window of time to ensure that
this isn't a problem going forward it seems prudent to go ahead do that
now.

> > Would packing this struct actually change the layout on either x86 or
> > x86_64?
> 
> Possibly, but we don't pack the other upcall/downcall arguments. See, for instance, the RPCSEC_GSS contexts.
> 

Right. It's possible that we'd never run afoul of this, but if we ever
do it won't be fun to straighten out. Will struct alignment look
different on 64-bit ARM, for instance? Who knows?

> > While we're looking at this...do we also need to worry about endianness
> >   here? Is it possible we'd ever end up running BE upcall code on a LE
> >   kernel (or vice versa) in some sort of horrid compat mode? If so, it
> >   might be worthwhile to consider making both those fields __be32 or
> >   something and fixing the code to handle that properly as well.
> > 
> > If we're going to go to that much trouble, I think I would ditch the binary
> > and go with text.  The upcall for block layout is not in a performance
> > critical path.
> 
> Urgh... If we're mixing endian modes, won't system calls be the first
> things to break?
> In any case, the current NFS client upcalls suppose that the endianness
> stays the same between kernel and userland.

Yeah, I'm just being anal here. Almost every binary interface in the
kernel relies on the endianness not changing so we're probably ok in
that regard.

I think the right thing to do is probably to go ahead and fix the
blocklayout upcall to use fixed-width types and packed structs before
3.1 ships (or text, or XDR...), and punt on the rest of them until we
find it to be a problem.

As Bruce points out, the idmap upcall is considered legacy now and at
some point in the future, I'd like to see the GSSAPI upcalls converted
to use the keys API. If that occurs, then those can just go away.

Cheers,
-- 
Jeff Layton <jlayton@redhat.com>

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[J. Bruce Fields]

On Fri, Sep 09, 2011 at 02:36:05PM -0400, Jeff Layton wrote:
> I've been looking at replacing the current scheme that knfsd uses to
> track client_id4's (aka the v4recoverydir stuff), with an
> upcall/downcall scheme. Primarily this is to allow for more robust
> handling of clustered NFSv4 services.
> 
> In the process, I've been looking at the various upcall schemes we use
> to see which ones might be suitable to use in this effort. I've noticed
> that we have several upcalls that use rpc_pipefs, and that all of them
> seem to make assumptions that the userspace programs will align their
> message structs identically to how the kernel does.
> 
> For instance, here's the idmap one:
> 
> struct idmap_msg {
>         __u8  im_type;
>         __u8  im_conv;
>         char  im_name[IDMAP_NAMESZ];
>         __u32 im_id;
>         __u8  im_status;
> };

That's the "legacy" idmap code, right?

In which case we want to leave it alone if at all possible and move
people to the new idmapper.

--b.

> 
> Note that this struct does not have __attribute__((packed)), so the
> compiler is allowed to add padding between the fields as it sees fit.
> 
> If, for instance, someone were to build the userspace programs
> differently than the kernel (for instance x86_64 kernel with i686
> userspace), it's possible that the padding between them would be
> different. It's also possible that different compilers might align
> things differently here.
> 
> The blocklayout upcall is even more scary as the width of the status
> field is not explicit:
> 
> struct bl_dev_msg {
>         int status;
>         uint32_t major, minor;
> };
> 
> ...it's unlikely that the kernel and userspace would differ on the size
> of an int here, but it might be a good idea to go ahead and make that
> explicitly 32 bits in case we end up dealing with more exotic arches at
> some point in the future.
> 
> I'm not sure what we can really do about this at this point. Adding
> this attribute now would definitely be an kernel/userspace
> compatibility issue.
> 
> One possibility is to add padding between the fields that aligns with
> the current padding that the compiler adds and then make them "packed".
> That might make these structs arch-specific though since different
> arches probably pad these differently... :-/
> 
> Am I making mountains out of molehills here? Thoughts?
> 
> -- 
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: shouldn't rpc_pipe_upcall message structs be __attribute__((packed)) ?

[Jeff Layton]

On Fri, 9 Sep 2011 16:03:04 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, Sep 09, 2011 at 02:36:05PM -0400, Jeff Layton wrote:
> > I've been looking at replacing the current scheme that knfsd uses to
> > track client_id4's (aka the v4recoverydir stuff), with an
> > upcall/downcall scheme. Primarily this is to allow for more robust
> > handling of clustered NFSv4 services.
> > 
> > In the process, I've been looking at the various upcall schemes we use
> > to see which ones might be suitable to use in this effort. I've noticed
> > that we have several upcalls that use rpc_pipefs, and that all of them
> > seem to make assumptions that the userspace programs will align their
> > message structs identically to how the kernel does.
> > 
> > For instance, here's the idmap one:
> > 
> > struct idmap_msg {
> >         __u8  im_type;
> >         __u8  im_conv;
> >         char  im_name[IDMAP_NAMESZ];
> >         __u32 im_id;
> >         __u8  im_status;
> > };
> 
> That's the "legacy" idmap code, right?
> 
> In which case we want to leave it alone if at all possible and move
> people to the new idmapper.
> 
> --b.
> 

Ahh good point. No need to sweat this one then.

-- 
Jeff Layton <jlayton@redhat.com>