Received-SPF: pass (google.com: domain of linux-nfs+bounces-3236-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <85041331-3fb7-4418-8aa2-2660ed99e85b@redhat.com>
Date: Fri, 10 May 2024 15:02:15 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Changing the precedence order of client exports in /etc/exports
To: James Pearson <jcpearson@gmail.com>, NeilBrown <neilb@suse.de>
Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <662a39f7.BuSJ6zaMPTMaMa7L%james-p@moving-picture.com>
 <CAK3fRr8N4dNz2+K-BgaZAcswbfXrDem6Z9fRtgTDMJa=Y0R8gA@mail.gmail.com>
 <171410661177.7600.9594587292479704884@noble.neil.brown.name>
 <CAK3fRr9kngizdGsAU8NpzBjpPSh4k6-23wpwfpZUHJeEoU7yVg@mail.gmail.com>
 <CAK3fRr_V02Gv2Vk4Ba5cyyRhtfwbnPeJ_xRmSrWcanLJRMEvaw@mail.gmail.com>
Content-Language: en-US
From: Steve Dickson <steved@redhat.com>
In-Reply-To: <CAK3fRr_V02Gv2Vk4Ba5cyyRhtfwbnPeJ_xRmSrWcanLJRMEvaw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 5/8/24 5:49 AM, James Pearson wrote:
> On Fri, 26 Apr 2024 at 17:08, James Pearson <jcpearson@gmail.com> wrote:
>>
>> On Fri, 26 Apr 2024 at 05:43, NeilBrown <neilb@suse.de> wrote:
>>>
>>> On Thu, 25 Apr 2024, James Pearson wrote:
>>>> Many years ago, I asked on this list if it was possible to change the
>>>> precedence order of exports listed in /etc/exports where there is more
>>>> than one possible match (see the thread at:
>>>> https://marc.info/?l=linux-nfs&m=130565040627856&w=2) - and answer was
>>>> 'No'
>>>>
>>>> At that time, I used a simple hack to force the precedence order I
>>>> required (by modifying the 'MCL' enum order in nfs-utils
>>>> support/include/exportfs.h)
>>>>
>>>> However, the issue has come up again for me, so I thought I would see
>>>> if I could alter the precedence order by adding an exports 'priority='
>>>> option as suggested later in the above thread
>>>>
>>>> I started with the nfs-utils supplied with CentOS 7 (based on 1.3.0) -
>>>> and added logic to lookup_export() to check for client specifications
>>>> with a higher priority - but this didn't work - so looking for other
>>>> places that looped through MCL types, I added similar logic in
>>>> nfsd_fh() - which seems to work as I expected (I'm using NFSv3)
>>>>
>>>> However, adding similar logic to the nfs-utils supplied with Rocky 9
>>>> (based on 2.5.4) didn't work ...
>>>>
>>>> But comparing the code in nfsd_fh() in v1.3.0 and nfsd_handle_fh() in
>>>> v2.5.4, nfsd_fh() in v1.3.0 does the following towards the end of the
>>>> function - whereas nfsd_handle_fh() in v2.5.4 doesn't:
>>>>
>>>>          if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
>>>>                  found = 0;
>>>>
>>>> By adding the above lines at a similar place in nfsd_handle_fh() in
>>>> v2.5.4, seems to 'fix' the issue and all works as I expected
>>>>
>>>> I don't fully understand what is going on under the hood with all
>>>> this, so no idea if what I've done is 'correct', or if there is a
>>>> better way of doing what I'm trying to achieve ?
>>>>
>>>> Below is a patch (made against the latest git nfs-utils) of what I've
>>>> done - could anyone let me know if I'm going along the right lines (or
>>>> not) ?
>>>
>>> The restored cache_export_ent() call has to go.
>>> You need to update init_exportent() to initialise the new field.
>>> You probably need to make some changes to auth_authenticate_newcache().
>>> Probably let the loop run all the way to MCL_MAXTYPES, and do a priority
>>> comparison if you find a new possible match.
>>> export_find() probably need some attention too.
>>>
>>> If you it still doesn't work after addressing those, I'll have a look
>>> and see if I can beat it into shape.
>>
>> Thanks for the pointers - new patch below
>>
>> I don't quite understand what export_find() is actually doing ?
>>
>> As far as I can tell, it is only used by exportfs when an export is
>> given on the command line - and only if that export is of type
>> MCL_FQDN - so I'm not sure it needs any changes to support these
>> priority additions ? (I might be completely wrong here ...)
> 
> Does this patch look OK ?
It needs a "Signed-off-by: Your name <your email>" line
To do that do a git commit -s in your cloned repos

Then a "git format-patch" to create the patch
which will add a "[PATCH]" to the subject line
with is something I filter on.

> 
> Does anything need to be added to export_find() ?
There needs to be an addition to the man
page explaining the new option and how it should
be used,

It is a lot of change to critical part of the export
code... How did you tested it?

steved.

> 
> Thanks
> 
> James Pearson
> 
>> diff --git a/support/export/auth.c b/support/export/auth.c
>> index 2d7960f1..3d9e07b5 100644
>> --- a/support/export/auth.c
>> +++ b/support/export/auth.c
>> @@ -175,7 +175,7 @@ auth_authenticate_newcache(const struct sockaddr *caller,
>>                             const char *path, struct addrinfo *ai,
>>                             enum auth_error *error)
>>   {
>> -       nfs_export *exp;
>> +       nfs_export *exp, *found;
>>          int i;
>>
>>          free(my_client.m_hostname);
>> @@ -189,6 +189,7 @@ auth_authenticate_newcache(const struct sockaddr *caller,
>>          my_exp.m_client = &my_client;
>>
>>          exp = NULL;
>> +       found = NULL;
>>          for (i = 0; !exp && i < MCL_MAXTYPES; i++)
>>                  for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
>>                          if (strcmp(path, exp->m_export.e_path))
>> @@ -198,8 +199,11 @@ auth_authenticate_newcache(const struct sockaddr *caller,
>>                          if (exp->m_export.e_flags & NFSEXP_V4ROOT)
>>                                  /* not acceptable for v[23] export */
>>                                  continue;
>> -                       break;
>> +                       /* we have a match - see if it is a higher priority */
>> +                       if (!found || exp->m_export.e_priority >
>> found->m_export.e_priority)
>> +                               found = exp;
>>                  }
>> +       exp = found;
>>          *error = not_exported;
>>          if (!exp)
>>                  return NULL;
>> diff --git a/support/export/cache.c b/support/export/cache.c
>> index 6c0a44a3..dfb0051b 100644
>> --- a/support/export/cache.c
>> +++ b/support/export/cache.c
>> @@ -877,6 +877,14 @@ static int nfsd_handle_fh(int f, char *bp, int blen)
>>                                  xlog(L_WARNING, "%s and %s have same
>> filehandle for %s, using first",
>>                                       found_path, path, dom);
>>                          } else {
>> +                               /* same path, see if this one has a
>> higher export priority */
>> +                               if (exp->m_export.e_priority >
>> found->e_priority) {
>> +                                       found = &exp->m_export;
>> +                                       free(found_path);
>> +                                       found_path = strdup(path);
>> +                                       if (found_path == NULL)
>> +                                               goto out;
>> +                               }
>>                                  /* same path, if one is V4ROOT, choose
>> the other */
>>                                  if (found->e_flags & NFSEXP_V4ROOT) {
>>                                          found = &exp->m_export;
>> @@ -1178,6 +1186,12 @@ lookup_export(char *dom, char *path, struct addrinfo *ai)
>>                                  found_type = i;
>>                                  continue;
>>                          }
>> +                       /* see if this one has a higher export priority */
>> +                       if (exp->m_export.e_priority >
>> found->m_export.e_priority) {
>> +                               found = exp;
>> +                               found_type = i;
>> +                               continue;
>> +                       }
>>                          /* Always prefer non-V4ROOT exports */
>>                          if (exp->m_export.e_flags & NFSEXP_V4ROOT)
>>                                  continue;
>> diff --git a/support/include/nfslib.h b/support/include/nfslib.h
>> index eff2a486..ab22ecaf 100644
>> --- a/support/include/nfslib.h
>> +++ b/support/include/nfslib.h
>> @@ -99,6 +99,7 @@ struct exportent {
>>          unsigned int    e_ttl;
>>          char *          e_realpath;
>>          int             e_reexport;
>> +       int             e_priority;
>>   };
>>
>>   struct rmtabent {
>> diff --git a/support/nfs/exports.c b/support/nfs/exports.c
>> index a6816e60..afc139db 100644
>> --- a/support/nfs/exports.c
>> +++ b/support/nfs/exports.c
>> @@ -106,6 +106,7 @@ static void init_exportent (struct exportent *ee,
>> int fromkernel)
>>          ee->e_uuid = NULL;
>>          ee->e_ttl = default_ttl;
>>          ee->e_reexport = REEXP_NONE;
>> +       ee->e_priority = 0;
>>   }
>>
>>   struct exportent *
>> @@ -374,6 +375,9 @@ putexportent(struct exportent *ep)
>>                                  fprintf(fp, "%d,", id[i]);
>>          }
>>          fprintf(fp, "anonuid=%d,anongid=%d", ep->e_anonuid, ep->e_anongid);
>> +       if (ep->e_priority) {
>> +               fprintf(fp, ",priority=%d", ep->e_priority);
>> +       }
>>          secinfo_show(fp, ep);
>>          xprtsecinfo_show(fp, ep);
>>          fprintf(fp, ")\n");
>> @@ -834,6 +838,14 @@ bad_option:
>>                                  setflags(NFSEXP_FSID, active, ep);
>>
>>                          saw_reexport = 1;
>> +               } else if (strncmp(opt, "priority=", 9) == 0) {
>> +                       char *oe;
>> +                       ep->e_priority = strtol(opt+9, &oe, 10);
>> +                       if (opt[9]=='\0' || *oe != '\0') {
>> +                               xlog(L_ERROR, "%s: %d: bad priority \"%s\"\n",
>> +                                    flname, flline, opt);
>> +                               goto bad_option;
>> +                       }
>>                  } else {
>>                          xlog(L_ERROR, "%s:%d: unknown keyword \"%s\"\n",
>>                                          flname, flline, opt);
>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>> index b03a047b..5e6a64b6 100644
>> --- a/utils/exportfs/exportfs.c
>> +++ b/utils/exportfs/exportfs.c
>> @@ -753,6 +753,8 @@ dump(int verbose, int export_format)
>>                                  break;
>>   #endif
>>                          }
>> +                       if (ep->e_priority)
>> +                               c = dumpopt(c, "priority=%d", ep->e_priority);
>>                          secinfo_show(stdout, ep);
>>                          xprtsecinfo_show(stdout, ep);
>>                          printf("%c\n", (c != '(')? ')' : ' ');
>