On Mon, 21 Mar 2022 14:12:31 +0000
Chuck Lever III <[email protected]> wrote:
couldn't find offender patch on ML so replying here
> Hi Linus-
>
> The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:
>
> Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18
>
> for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:
>
> nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)
>
> ----------------------------------------------------------------
> New features:
> - NFSv3 support in NFSD is now always built
> - Added NFSD support for the NFSv4 birth-time file attribute
[...]
> Ondrej Valousek (1):
> nfsd: Add support for the birth time attribute
This patch regressed clients that support TIME_CREATE attribute.
Starting with this patch client might think that server supports
TIME_CREATE and start sending this attribute in its requests.
However kernel on server side (since this patch and to
current master) upon getting such request will return EINVAL.
(my guess is that TIME_CREATE not being decoded properly and
that messes up request parsing).
End result is unusable mount (unless it's treated as readonly).
Reproduces with current master (HEAD at e5524c2a1fc40) and MacOS
client (Big Sur or newest Monterey).
server is typical setup exporting files from XFS (Fedora36)
# rpcdebug -m nfsd -s all
on client:
% mount -t nfs -o vers=4,rw,nfc,sec=sys testnas:/mnt ~/test
% touch ~/test/fff
touch: test/fff: Invalid argument
server logs:
nfsd: fh_compose(exp fd:00/128 fff, ino=0)
NFSD: nfsd4_open filename op_openowner 0000000000000000
Here is a request the touch generates:
Network File System, Ops(6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
[Program Version: 4]
[V4 Procedure: COMPOUND (1)]
Tag: create
minorversion: 0
Operations (count: 6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
Opcode: PUTFH (22)
Opcode: SAVEFH (32)
Opcode: OPEN (18)
seqid: 0x00000004
share_access: OPEN4_SHARE_ACCESS_BOTH (3)
share_deny: OPEN4_SHARE_DENY_NONE (0)
clientid: 0xba93c9620aec46ea
owner: <DATA>
Open Type: OPEN4_CREATE (1)
Create Mode: UNCHECKED4 (0)
Attr mask: 0x00040002 (Mode, Time_Create)
reco_attr: Mode (33)
reco_attr: Time_Create (50)
Claim Type: CLAIM_NULL (0)
Name: fff
[...]
when trying to copy file via GUI (Finder) it goes a different route
but ends up with error anyway and with leftover 0-length file on server
with messed up permissions, i.e.
open/create without Time_Create succeeds but followup
setattr with Time_Create fails EINVAL.
Network File System, Ops(3): PUTFH, SETATTR, GETATTR
[Program Version: 4]
[V4 Procedure: COMPOUND (1)]
Tag: setattr
minorversion: 0
Operations (count: 3): PUTFH, SETATTR, GETATTR
Opcode: PUTFH (22)
Opcode: SETATTR (34)
StateID
Attr mask: 0x00450002 (Mode, Time_Access_Set, Time_Create, Time_Modify_Set)
reco_attr: Mode (33)
reco_attr: Time_Access_Set (48)
reco_attr: Time_Create (50)
reco_attr: Time_Modify_Set (54)
Opcode: GETATTR (9)
[Main Opcode: SETATTR (34)]
[...]
> --
> Chuck Lever
>
>
>
> On Jul 10, 2022, at 6:43 AM, Igor Mammedov <[email protected]> wrote:
>
> On Mon, 21 Mar 2022 14:12:31 +0000
> Chuck Lever III <[email protected]> wrote:
>
> couldn't find offender patch on ML so replying here
Probably:
https://lore.kernel.org/linux-nfs/[email protected]/
>> Hi Linus-
>>
>> The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:
>>
>> Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)
>>
>> are available in the Git repository at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18
>>
>> for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:
>>
>> nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)
>>
>> ----------------------------------------------------------------
>> New features:
>> - NFSv3 support in NFSD is now always built
>> - Added NFSD support for the NFSv4 birth-time file attribute
> [...]
>
>> Ondrej Valousek (1):
>> nfsd: Add support for the birth time attribute
Thank you for the report, Igor.
> This patch regressed clients that support TIME_CREATE attribute.
> Starting with this patch client might think that server supports
> TIME_CREATE and start sending this attribute in its requests.
Indeed, e377a3e698fb ("nfsd: Add support for the birth time
attribute") does not include a change to nfsd4_decode_fattr4()
that decodes the birth time attribute.
I don't immediately see another storage protocol stack in our
kernel that supports a client setting the birth time, so NFSD
might have to ignore the client-provided value.
> However kernel on server side (since this patch and to
> current master) upon getting such request will return EINVAL.
> (my guess is that TIME_CREATE not being decoded properly and
> that messes up request parsing).
I'll send a quick-and-dirty fix your way as we explore the
question of whether NFSD needs to ignore the birth time value
in this case.
> End result is unusable mount (unless it's treated as readonly).
That seems odd, and not clear whether that's a client or server
problem. I hope that will clear up once the server deals with
the time_create attribute appropriately.
> Reproduces with current master (HEAD at e5524c2a1fc40) and MacOS
> client (Big Sur or newest Monterey).
>
> server is typical setup exporting files from XFS (Fedora36)
>
> # rpcdebug -m nfsd -s all
>
> on client:
>
> % mount -t nfs -o vers=4,rw,nfc,sec=sys testnas:/mnt ~/test
> % touch ~/test/fff
> touch: test/fff: Invalid argument
>
> server logs:
>
> nfsd: fh_compose(exp fd:00/128 fff, ino=0)
> NFSD: nfsd4_open filename op_openowner 0000000000000000
>
> Here is a request the touch generates:
> Network File System, Ops(6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
> [Program Version: 4]
> [V4 Procedure: COMPOUND (1)]
> Tag: create
> minorversion: 0
> Operations (count: 6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
> Opcode: PUTFH (22)
> Opcode: SAVEFH (32)
> Opcode: OPEN (18)
> seqid: 0x00000004
> share_access: OPEN4_SHARE_ACCESS_BOTH (3)
> share_deny: OPEN4_SHARE_DENY_NONE (0)
> clientid: 0xba93c9620aec46ea
> owner: <DATA>
> Open Type: OPEN4_CREATE (1)
> Create Mode: UNCHECKED4 (0)
> Attr mask: 0x00040002 (Mode, Time_Create)
> reco_attr: Mode (33)
> reco_attr: Time_Create (50)
> Claim Type: CLAIM_NULL (0)
> Name: fff
>
> [...]
>
> when trying to copy file via GUI (Finder) it goes a different route
> but ends up with error anyway and with leftover 0-length file on server
> with messed up permissions, i.e.
The current NFSv4 OPEN(CREATE) code path is still not right. Fixing
the TIME_CREATE problem should make this symptom go away for now,
but eventually that path will need to be restructured so that it
cannot leave a turd if the whole create process was not able to
complete.
> open/create without Time_Create succeeds but followup
> setattr with Time_Create fails EINVAL.
>
> Network File System, Ops(3): PUTFH, SETATTR, GETATTR
> [Program Version: 4]
> [V4 Procedure: COMPOUND (1)]
> Tag: setattr
> minorversion: 0
> Operations (count: 3): PUTFH, SETATTR, GETATTR
> Opcode: PUTFH (22)
> Opcode: SETATTR (34)
> StateID
> Attr mask: 0x00450002 (Mode, Time_Access_Set, Time_Create, Time_Modify_Set)
> reco_attr: Mode (33)
> reco_attr: Time_Access_Set (48)
> reco_attr: Time_Create (50)
> reco_attr: Time_Modify_Set (54)
> Opcode: GETATTR (9)
> [Main Opcode: SETATTR (34)]
>
> [...]
>> --
>> Chuck Lever
--
Chuck Lever
On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
>
> > On Jul 10, 2022, at 6:43 AM, Igor Mammedov <[email protected]> wrote:
> >
> > On Mon, 21 Mar 2022 14:12:31 +0000
> > Chuck Lever III <[email protected]> wrote:
> >
> > couldn't find offender patch on ML so replying here
>
> Probably:
>
> https://lore.kernel.org/linux-nfs/[email protected]/
>
>
> > > Hi Linus-
> > >
> > > The following changes since commit 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3:
> > >
> > > Linux 5.17-rc6 (2022-02-27 14:36:33 -0800)
> > >
> > > are available in the Git repository at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-5.18
> > >
> > > for you to fetch changes up to 4fc5f5346592cdc91689455d83885b0af65d71b8:
> > >
> > > nfsd: fix using the correct variable for sizeof() (2022-03-20 12:49:38 -0400)
> > >
> > > ----------------------------------------------------------------
> > > New features:
> > > - NFSv3 support in NFSD is now always built
> > > - Added NFSD support for the NFSv4 birth-time file attribute
> > [...]
> >
> > > Ondrej Valousek (1):
> > > nfsd: Add support for the birth time attribute
>
> Thank you for the report, Igor.
>
>
> > This patch regressed clients that support TIME_CREATE attribute.
> > Starting with this patch client might think that server supports
> > TIME_CREATE and start sending this attribute in its requests.
>
> Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> attribute") does not include a change to nfsd4_decode_fattr4()
> that decodes the birth time attribute.
>
> I don't immediately see another storage protocol stack in our
> kernel that supports a client setting the birth time, so NFSD
> might have to ignore the client-provided value.
>
Cephfs allows this. My thinking at the time that I implemented it was
that it should be settable for backup purposes, but this was possibly a
mistake. On most filesystems, the btime seems to be equivalent to inode
creation time and is read-only.
>
> > However kernel on server side (since this patch and to
> > current master) upon getting such request will return EINVAL.
> > (my guess is that TIME_CREATE not being decoded properly and
> > that messes up request parsing).
>
> I'll send a quick-and-dirty fix your way as we explore the
> question of whether NFSD needs to ignore the birth time value
> in this case.
>
>
> > End result is unusable mount (unless it's treated as readonly).
>
> That seems odd, and not clear whether that's a client or server
> problem. I hope that will clear up once the server deals with
> the time_create attribute appropriately.
>
>
> > Reproduces with current master (HEAD at e5524c2a1fc40) and MacOS
> > client (Big Sur or newest Monterey).
> >
> > server is typical setup exporting files from XFS (Fedora36)
> >
> > # rpcdebug -m nfsd -s all
> >
> > on client:
> >
> > % mount -t nfs -o vers=4,rw,nfc,sec=sys testnas:/mnt ~/test
> > % touch ~/test/fff
> > touch: test/fff: Invalid argument
> >
> > server logs:
> >
> > nfsd: fh_compose(exp fd:00/128 fff, ino=0)
> > NFSD: nfsd4_open filename op_openowner 0000000000000000
> >
> > Here is a request the touch generates:
> > Network File System, Ops(6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
> > [Program Version: 4]
> > [V4 Procedure: COMPOUND (1)]
> > Tag: create
> > minorversion: 0
> > Operations (count: 6): PUTFH, SAVEFH, OPEN, GETATTR, RESTOREFH, GETATTR
> > Opcode: PUTFH (22)
> > Opcode: SAVEFH (32)
> > Opcode: OPEN (18)
> > seqid: 0x00000004
> > share_access: OPEN4_SHARE_ACCESS_BOTH (3)
> > share_deny: OPEN4_SHARE_DENY_NONE (0)
> > clientid: 0xba93c9620aec46ea
> > owner: <DATA>
> > Open Type: OPEN4_CREATE (1)
> > Create Mode: UNCHECKED4 (0)
> > Attr mask: 0x00040002 (Mode, Time_Create)
> > reco_attr: Mode (33)
> > reco_attr: Time_Create (50)
> > Claim Type: CLAIM_NULL (0)
> > Name: fff
> >
> > [...]
> >
> > when trying to copy file via GUI (Finder) it goes a different route
> > but ends up with error anyway and with leftover 0-length file on server
> > with messed up permissions, i.e.
>
> The current NFSv4 OPEN(CREATE) code path is still not right. Fixing
> the TIME_CREATE problem should make this symptom go away for now,
> but eventually that path will need to be restructured so that it
> cannot leave a turd if the whole create process was not able to
> complete.
>
>
> > open/create without Time_Create succeeds but followup
> > setattr with Time_Create fails EINVAL.
> >
> > Network File System, Ops(3): PUTFH, SETATTR, GETATTR
> > [Program Version: 4]
> > [V4 Procedure: COMPOUND (1)]
> > Tag: setattr
> > minorversion: 0
> > Operations (count: 3): PUTFH, SETATTR, GETATTR
> > Opcode: PUTFH (22)
> > Opcode: SETATTR (34)
> > StateID
> > Attr mask: 0x00450002 (Mode, Time_Access_Set, Time_Create, Time_Modify_Set)
> > reco_attr: Mode (33)
> > reco_attr: Time_Access_Set (48)
> > reco_attr: Time_Create (50)
> > reco_attr: Time_Modify_Set (54)
> > Opcode: GETATTR (9)
> > [Main Opcode: SETATTR (34)]
> >
> > [...]
> > > --
> > > Chuck Lever
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > This patch regressed clients that support TIME_CREATE attribute.
> > > Starting with this patch client might think that server supports
> > > TIME_CREATE and start sending this attribute in its requests.
> >
> > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > attribute") does not include a change to nfsd4_decode_fattr4()
> > that decodes the birth time attribute.
> >
> > I don't immediately see another storage protocol stack in our
> > kernel that supports a client setting the birth time, so NFSD
> > might have to ignore the client-provided value.
> >
>
> Cephfs allows this. My thinking at the time that I implemented it was
> that it should be settable for backup purposes, but this was possibly a
> mistake. On most filesystems, the btime seems to be equivalent to inode
> creation time and is read-only.
So supporting it as read-only seems reasonable.
Clearly, failing to decode the setattr attempt isn't the right way to do
that. I'm not sure what exactly it should be doing--some kind of
permission error on any setattr containing TIME_CREATE?
--b.
> On Jul 11, 2022, at 2:19 PM, Bruce Fields <[email protected]> wrote:
>
> On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
>> On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
>>>> This patch regressed clients that support TIME_CREATE attribute.
>>>> Starting with this patch client might think that server supports
>>>> TIME_CREATE and start sending this attribute in its requests.
>>>
>>> Indeed, e377a3e698fb ("nfsd: Add support for the birth time
>>> attribute") does not include a change to nfsd4_decode_fattr4()
>>> that decodes the birth time attribute.
>>>
>>> I don't immediately see another storage protocol stack in our
>>> kernel that supports a client setting the birth time, so NFSD
>>> might have to ignore the client-provided value.
>>>
>>
>> Cephfs allows this. My thinking at the time that I implemented it was
>> that it should be settable for backup purposes, but this was possibly a
>> mistake. On most filesystems, the btime seems to be equivalent to inode
>> creation time and is read-only.
>
> So supporting it as read-only seems reasonable.
>
> Clearly, failing to decode the setattr attempt isn't the right way to do
> that. I'm not sure what exactly it should be doing--some kind of
> permission error on any setattr containing TIME_CREATE?
I don't think that will work.
NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
the mask of attributes it supports. That means the server has
to process GETATTR and SETATTR (and OPEN) operations that
contain FATTR4_WORD1_TIME_CREATE as not an error. The protocol
allows the server to indicate it ignored the time_create value
by clearing the FATTR4_WORD1_TIME_CREATE bit in the attribute
bitmask it returns in the reply.
--
Chuck Lever
On Mon, 11 Jul 2022 14:19:41 -0400
Bruce Fields <[email protected]> wrote:
> On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > Starting with this patch client might think that server supports
> > > > TIME_CREATE and start sending this attribute in its requests.
> > >
> > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > that decodes the birth time attribute.
> > >
> > > I don't immediately see another storage protocol stack in our
> > > kernel that supports a client setting the birth time, so NFSD
> > > might have to ignore the client-provided value.
> > >
> >
> > Cephfs allows this. My thinking at the time that I implemented it was
> > that it should be settable for backup purposes, but this was possibly a
> > mistake. On most filesystems, the btime seems to be equivalent to inode
> > creation time and is read-only.
>
> So supporting it as read-only seems reasonable.
>
> Clearly, failing to decode the setattr attempt isn't the right way to do
> that. I'm not sure what exactly it should be doing--some kind of
> permission error on any setattr containing TIME_CREATE?
erroring out on TIME_CREATE will break client that try to
set this attribute (legitimately). That's what by chance
happening with current master (return error when TIME_CREATE
is present).
As long as server advertises support for TIME_CREATE
it should not error out when client sends it if spec permits
such use.
I think ignoring this attribute like Chuck has proposed
is acceptable (if one ignores archiving use case where
setting it makes sense).
Alternatively if folks inclined towards erroring out,
there should be a way to optout or optin from TIME_CREATE support,
to keep existing clients working + a sane error message so users
won't have to debug kernel to figure out what's wrong with
their setup.
> --b.
>
On Tue, Jul 12, 2022 at 10:27:46AM +0200, Igor Mammedov wrote:
> On Mon, 11 Jul 2022 14:19:41 -0400
> Bruce Fields <[email protected]> wrote:
>
> > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > Starting with this patch client might think that server supports
> > > > > TIME_CREATE and start sending this attribute in its requests.
> > > >
> > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > that decodes the birth time attribute.
> > > >
> > > > I don't immediately see another storage protocol stack in our
> > > > kernel that supports a client setting the birth time, so NFSD
> > > > might have to ignore the client-provided value.
> > > >
> > >
> > > Cephfs allows this. My thinking at the time that I implemented it was
> > > that it should be settable for backup purposes, but this was possibly a
> > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > creation time and is read-only.
> >
> > So supporting it as read-only seems reasonable.
> >
> > Clearly, failing to decode the setattr attempt isn't the right way to do
> > that. I'm not sure what exactly it should be doing--some kind of
> > permission error on any setattr containing TIME_CREATE?
>
> erroring out on TIME_CREATE will break client that try to
> set this attribute (legitimately). That's what by chance
> happening with current master (return error when TIME_CREATE
> is present).
Hang on, now--our current server completely fails to decode any RPC
including a SETATTR that attempts to set TIME_CREATE, which means it
isn't able to return a useful error or tell the client which attribute
was the problem.
It's not too surprising that that would cause a problem for a client.
But failures to set supported attributes are completely normal, and if
mounts are failing completely because of that, something is really very
wrong with the client.
Could you first retest with a server that's patched to at least decode
the attribute correctly? I suspect that may be enough. If not, then
the client in question has a more interesting problem on its hands.
--b.
On Tue, 12 Jul 2022 07:42:11 -0400
Bruce Fields <[email protected]> wrote:
> On Tue, Jul 12, 2022 at 10:27:46AM +0200, Igor Mammedov wrote:
> > On Mon, 11 Jul 2022 14:19:41 -0400
> > Bruce Fields <[email protected]> wrote:
> >
> > > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > > Starting with this patch client might think that server supports
> > > > > > TIME_CREATE and start sending this attribute in its requests.
> > > > >
> > > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > > that decodes the birth time attribute.
> > > > >
> > > > > I don't immediately see another storage protocol stack in our
> > > > > kernel that supports a client setting the birth time, so NFSD
> > > > > might have to ignore the client-provided value.
> > > > >
> > > >
> > > > Cephfs allows this. My thinking at the time that I implemented it was
> > > > that it should be settable for backup purposes, but this was possibly a
> > > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > > creation time and is read-only.
> > >
> > > So supporting it as read-only seems reasonable.
> > >
> > > Clearly, failing to decode the setattr attempt isn't the right way to do
> > > that. I'm not sure what exactly it should be doing--some kind of
> > > permission error on any setattr containing TIME_CREATE?
> >
> > erroring out on TIME_CREATE will break client that try to
> > set this attribute (legitimately). That's what by chance
> > happening with current master (return error when TIME_CREATE
> > is present).
>
> Hang on, now--our current server completely fails to decode any RPC
> including a SETATTR that attempts to set TIME_CREATE, which means it
> isn't able to return a useful error or tell the client which attribute
> was the problem.
>
> It's not too surprising that that would cause a problem for a client.
>
> But failures to set supported attributes are completely normal, and if
> mounts are failing completely because of that, something is really very
> wrong with the client.
returning unsupported attribute error might work, but as Chuck mentioned
we do kind of support TIME_CREATE for some requests so client might be
confused when server itself sends this attribute while errors out when
client tries to send it.
What I'm saying if we are to try returning error in this case
it should be tested with variety of clients before committing
to this approach. (meanwhile decoding and ignoring attribute
with Chuck's patch fixes immediate issue).
> Could you first retest with a server that's patched to at least decode
> the attribute correctly? I suspect that may be enough. If not, then
it does work with fixed decoding path:
(i.e. patched with https://lore.kernel.org/lkml/[email protected]/T/)
> the client in question has a more interesting problem on its hands.
>
> --b.
>