[PATCH 0/2] Two fixes related to '--concurrent'.

[PATCH 0/2] Two fixes related to '--concurrent'.

[Firo Yang]

Fixed two problems related to '--concurrent'.

Firo Yang (2):
  ebtables: processing '--concurrent' beofore other arguments
  libebtc: Fix an issue that '--concurrent' doesn't work with NFS

 ebtables.c | 22 +++++++++++++++++++---
 libebtc.c  |  2 +-
 2 files changed, 20 insertions(+), 4 deletions(-)


base-commit: 46eb78ff358724f5addf14e45f2cfc31542ede3c
-- 
2.30.2

[PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Firo Yang]

Our customer reported a following issue:
If '--concurrent' was passed to ebtables command behind other arguments,
'--concurrent' will not take effect sometimes; for a simple example,
ebtables -L --concurrent. This is becuase the handling of '--concurrent'
is implemented in a passing-order-dependent way.

So we can fix this problem by processing it before other arguments.

Signed-off-by: Firo Yang <firo.yang@suse.com>
---
 ebtables.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/ebtables.c b/ebtables.c
index f7dfccf..98f655b 100644
--- a/ebtables.c
+++ b/ebtables.c
@@ -98,6 +98,12 @@ static struct option ebt_original_options[] =
 
 static struct option *ebt_options = ebt_original_options;
 
+static struct option ebt_global_options[] =
+{
+        { "concurrent"     , no_argument      , 0, 13  },
+        { 0 }
+};
+
 /* Holds all the data */
 static struct ebt_u_replace *replace;
 
@@ -580,6 +586,17 @@ int do_command(int argc, char *argv[], int exec_style,
 	 * '-t'  ,'-M' and --atomic (if specified) have to come
 	 * before '-A' and the like */
 
+	while ((c = getopt_long(argc, argv, "", ebt_global_options, NULL)) != -1) {
+		switch (c) {
+		case 13 : /* concurrent */
+                        use_lockfd = 1;
+                        break;
+                default :
+                        continue;
+                }
+        }
+
+        optind = 1;
 	/* Getopt saves the day */
 	while ((c = getopt_long(argc, argv,
 	   "-A:D:C:I:N:E:X::L::Z::F::P:Vhi:o:j:c:p:s:d:t:M:", ebt_options, NULL)) != -1) {
@@ -1040,9 +1057,8 @@ big_iface_length:
 			replace->filename = (char *)malloc(strlen(optarg) + 1);
 			strcpy(replace->filename, optarg);
 			break;
-		case 13 : /* concurrent */
-			use_lockfd = 1;
-			break;
+		case 13 :
+			continue;
 		case 1 :
 			if (!strcmp(optarg, "!"))
 				ebt_check_inverse2(optarg);
-- 
2.30.2

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Pablo Neira Ayuso]

Hi,

On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> Our customer reported a following issue:
> If '--concurrent' was passed to ebtables command behind other arguments,
> '--concurrent' will not take effect sometimes; for a simple example,
> ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> is implemented in a passing-order-dependent way.
> 
> So we can fix this problem by processing it before other arguments.

Would you instead make a patch to spew an error if --concurrent is the
first argument?

--concurrent has never worked unless you place it in first place
anyway.

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Pablo Neira Ayuso]

On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> > Our customer reported a following issue:
> > If '--concurrent' was passed to ebtables command behind other arguments,
> > '--concurrent' will not take effect sometimes; for a simple example,
> > ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> > is implemented in a passing-order-dependent way.
> > 
> > So we can fix this problem by processing it before other arguments.
> 
> Would you instead make a patch to spew an error if --concurrent is the
> first argument?

Wrong wording:

Would you instead make a patch to spew an error if --concurrent is
_not_ the first argument?

> --concurrent has never worked unless you place it in first place
> anyway.

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Firo Yang]

The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
> On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
> > Hi,
> > 
> > On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> > > Our customer reported a following issue:
> > > If '--concurrent' was passed to ebtables command behind other arguments,
> > > '--concurrent' will not take effect sometimes; for a simple example,
> > > ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> > > is implemented in a passing-order-dependent way.
> > > 
> > > So we can fix this problem by processing it before other arguments.
> > 
> > Would you instead make a patch to spew an error if --concurrent is the
> > first argument?
> 
> Wrong wording:
> 
> Would you instead make a patch to spew an error if --concurrent is
> _not_ the first argument?

Hi Pablo, I think it would make more sense if we don't introduce this
inconvenice to users. If you insist, I would go create the patch as you
intended.

--
Firo

> 
> > --concurrent has never worked unless you place it in first place
> > anyway.
> 

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Pablo Neira Ayuso]

On Tue, Apr 06, 2021 at 10:57:37AM +0800, Firo Yang wrote:
> The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
> > On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
> > > Hi,
> > >
> > > On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> > > > Our customer reported a following issue:
> > > > If '--concurrent' was passed to ebtables command behind other arguments,
> > > > '--concurrent' will not take effect sometimes; for a simple example,
> > > > ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> > > > is implemented in a passing-order-dependent way.
> > > >
> > > > So we can fix this problem by processing it before other arguments.
> > >
> > > Would you instead make a patch to spew an error if --concurrent is the
> > > first argument?
> >
> > Wrong wording:
> >
> > Would you instead make a patch to spew an error if --concurrent is
> > _not_ the first argument?
>
> Hi Pablo, I think it would make more sense if we don't introduce this
> inconvenice to users. If you insist, I would go create the patch as you
> intended.

See ebtables.c line 579.

        /* The scenario induced by this loop makes that:
         * '-t'  ,'-M' and --atomic (if specified) have to come
         * before '-A' and the like */

There are other options following this approach, please align
--concurrent with those.

Have a look at 'M' and 't' for the error that is reported.

                case 'M': /* Modprobe */
                        if (OPT_COMMANDS)
                                ebt_print_error2("Please put the -M option earlier");

Thanks.

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Simon Lees]

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



On 4/6/21 12:27 PM, Firo Yang wrote:
> The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
>> On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
>>> Hi,
>>>
>>> On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
>>>> Our customer reported a following issue:
>>>> If '--concurrent' was passed to ebtables command behind other arguments,
>>>> '--concurrent' will not take effect sometimes; for a simple example,
>>>> ebtables -L --concurrent. This is becuase the handling of '--concurrent'
>>>> is implemented in a passing-order-dependent way.
>>>>
>>>> So we can fix this problem by processing it before other arguments.
>>>
>>> Would you instead make a patch to spew an error if --concurrent is the
>>> first argument?
>>
>> Wrong wording:
>>
>> Would you instead make a patch to spew an error if --concurrent is
>> _not_ the first argument?
> 
> Hi Pablo, I think it would make more sense if we don't introduce this
> inconvenice to users. If you insist, I would go create the patch as you
> intended.

Agreed, that also wouldn't be seen as a workable solution for us "SUSE"
as our customers who may have scripts or documented processes where
--concurrent is not first and such a change would be considered a
"Change in behavior" as such we can't ship it in a bugfix or minor
version update, only in the next major update and we don't know when
that will be yet.

Sure this is probably only a issue for enterprise distro's but such a
change would likely inconvenience other users as well.

Cheers

-- 
Simon Lees (Simotek)                            http://simotek.net

Emergency Update Team                           keybase.io/simotek
SUSE Linux                           Adelaide Australia, UTC+10:30
GPG Fingerprint: 5B87 DB9D 88DC F606 E489 CEC5 0922 C246 02F0 014B


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 491 bytes --]

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

Hi,

On Tue, Apr 06, 2021 at 05:29:11PM +0930, Simon Lees wrote:
> 
> 
> On 4/6/21 12:27 PM, Firo Yang wrote:
> > The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
> >> On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
> >>> Hi,
> >>>
> >>> On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> >>>> Our customer reported a following issue:
> >>>> If '--concurrent' was passed to ebtables command behind other arguments,
> >>>> '--concurrent' will not take effect sometimes; for a simple example,
> >>>> ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> >>>> is implemented in a passing-order-dependent way.
> >>>>
> >>>> So we can fix this problem by processing it before other arguments.
> >>>
> >>> Would you instead make a patch to spew an error if --concurrent is the
> >>> first argument?
> >>
> >> Wrong wording:
> >>
> >> Would you instead make a patch to spew an error if --concurrent is
> >> _not_ the first argument?
> > 
> > Hi Pablo, I think it would make more sense if we don't introduce this
> > inconvenice to users. If you insist, I would go create the patch as you
> > intended.
> 
> Agreed, that also wouldn't be seen as a workable solution for us "SUSE"
> as our customers who may have scripts or documented processes where
> --concurrent is not first and such a change would be considered a
> "Change in behavior" as such we can't ship it in a bugfix or minor
> version update, only in the next major update and we don't know when
> that will be yet.
>
> Sure this is probably only a issue for enterprise distro's but such a
> change would likely inconvenience other users as well.

--concurrent has never worked away from the early positions ever.

What's the issue?

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

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Simon Lees]

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



On 4/6/21 7:22 PM, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Tue, Apr 06, 2021 at 05:29:11PM +0930, Simon Lees wrote:
>>
>>
>> On 4/6/21 12:27 PM, Firo Yang wrote:
>>> The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
>>>> On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
>>>>>> Our customer reported a following issue:
>>>>>> If '--concurrent' was passed to ebtables command behind other arguments,
>>>>>> '--concurrent' will not take effect sometimes; for a simple example,
>>>>>> ebtables -L --concurrent. This is becuase the handling of '--concurrent'
>>>>>> is implemented in a passing-order-dependent way.
>>>>>>
>>>>>> So we can fix this problem by processing it before other arguments.
>>>>>
>>>>> Would you instead make a patch to spew an error if --concurrent is the
>>>>> first argument?
>>>>
>>>> Wrong wording:
>>>>
>>>> Would you instead make a patch to spew an error if --concurrent is
>>>> _not_ the first argument?
>>>
>>> Hi Pablo, I think it would make more sense if we don't introduce this
>>> inconvenice to users. If you insist, I would go create the patch as you
>>> intended.
>>
>> Agreed, that also wouldn't be seen as a workable solution for us "SUSE"
>> as our customers who may have scripts or documented processes where
>> --concurrent is not first and such a change would be considered a
>> "Change in behavior" as such we can't ship it in a bugfix or minor
>> version update, only in the next major update and we don't know when
>> that will be yet.
>>
>> Sure this is probably only a issue for enterprise distro's but such a
>> change would likely inconvenience other users as well.
> 
> --concurrent has never worked away from the early positions ever.
> 
> What's the issue?

We had a customer complaining about the change in ordering causing
different results with one way working and the other not, looking back
at the report a second time I don't think they were ever using the "non
working way" in production but just to debug the other issue.

-- 
Simon Lees (Simotek)                            http://simotek.net

Emergency Update Team                           keybase.io/simotek
SUSE Linux                           Adelaide Australia, UTC+10:30
GPG Fingerprint: 5B87 DB9D 88DC F606 E489 CEC5 0922 C246 02F0 014B


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Tue, Apr 06, 2021 at 08:51:30PM +0930, Simon Lees wrote:
> 
> 
> On 4/6/21 7:22 PM, Pablo Neira Ayuso wrote:
> > Hi,
> > 
> > On Tue, Apr 06, 2021 at 05:29:11PM +0930, Simon Lees wrote:
> >>
> >>
> >> On 4/6/21 12:27 PM, Firo Yang wrote:
> >>> The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
> >>>> On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> >>>>>> Our customer reported a following issue:
> >>>>>> If '--concurrent' was passed to ebtables command behind other arguments,
> >>>>>> '--concurrent' will not take effect sometimes; for a simple example,
> >>>>>> ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> >>>>>> is implemented in a passing-order-dependent way.
> >>>>>>
> >>>>>> So we can fix this problem by processing it before other arguments.
> >>>>>
> >>>>> Would you instead make a patch to spew an error if --concurrent is the
> >>>>> first argument?
> >>>>
> >>>> Wrong wording:
> >>>>
> >>>> Would you instead make a patch to spew an error if --concurrent is
> >>>> _not_ the first argument?
> >>>
> >>> Hi Pablo, I think it would make more sense if we don't introduce this
> >>> inconvenice to users. If you insist, I would go create the patch as you
> >>> intended.
> >>
> >> Agreed, that also wouldn't be seen as a workable solution for us "SUSE"
> >> as our customers who may have scripts or documented processes where
> >> --concurrent is not first and such a change would be considered a
> >> "Change in behavior" as such we can't ship it in a bugfix or minor
> >> version update, only in the next major update and we don't know when
> >> that will be yet.
> >>
> >> Sure this is probably only a issue for enterprise distro's but such a
> >> change would likely inconvenience other users as well.
> > 
> > --concurrent has never worked away from the early positions ever.
> > 
> > What's the issue?
> 
> We had a customer complaining about the change in ordering causing
> different results with one way working and the other not, looking back
> at the report a second time I don't think they were ever using the "non
> working way" in production but just to debug the other issue.

Thanks for explaining, then I think we can go for the "restrict
position" fix which aligns with the -M, -t, ..., correct?

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

Re: [PATCH 1/2] ebtables: processing '--concurrent' beofore other arguments

[Firo Yang]

The 04/06/2021 13:31, Pablo Neira Ayuso wrote:
> On Tue, Apr 06, 2021 at 08:51:30PM +0930, Simon Lees wrote:
> > 
> > 
> > On 4/6/21 7:22 PM, Pablo Neira Ayuso wrote:
> > > Hi,
> > > 
> > > On Tue, Apr 06, 2021 at 05:29:11PM +0930, Simon Lees wrote:
> > >>
> > >>
> > >> On 4/6/21 12:27 PM, Firo Yang wrote:
> > >>> The 04/03/2021 20:22, Pablo Neira Ayuso wrote:
> > >>>> On Sat, Apr 03, 2021 at 08:15:17PM +0200, Pablo Neira Ayuso wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Thu, Apr 01, 2021 at 12:07:40PM +0800, Firo Yang wrote:
> > >>>>>> Our customer reported a following issue:
> > >>>>>> If '--concurrent' was passed to ebtables command behind other arguments,
> > >>>>>> '--concurrent' will not take effect sometimes; for a simple example,
> > >>>>>> ebtables -L --concurrent. This is becuase the handling of '--concurrent'
> > >>>>>> is implemented in a passing-order-dependent way.
> > >>>>>>
> > >>>>>> So we can fix this problem by processing it before other arguments.
> > >>>>>
> > >>>>> Would you instead make a patch to spew an error if --concurrent is the
> > >>>>> first argument?
> > >>>>
> > >>>> Wrong wording:
> > >>>>
> > >>>> Would you instead make a patch to spew an error if --concurrent is
> > >>>> _not_ the first argument?
> > >>>
> > >>> Hi Pablo, I think it would make more sense if we don't introduce this
> > >>> inconvenice to users. If you insist, I would go create the patch as you
> > >>> intended.
> > >>
> > >> Agreed, that also wouldn't be seen as a workable solution for us "SUSE"
> > >> as our customers who may have scripts or documented processes where
> > >> --concurrent is not first and such a change would be considered a
> > >> "Change in behavior" as such we can't ship it in a bugfix or minor
> > >> version update, only in the next major update and we don't know when
> > >> that will be yet.
> > >>
> > >> Sure this is probably only a issue for enterprise distro's but such a
> > >> change would likely inconvenience other users as well.
> > > 
> > > --concurrent has never worked away from the early positions ever.
> > > 
> > > What's the issue?
> > 
> > We had a customer complaining about the change in ordering causing
> > different results with one way working and the other not, looking back
> > at the report a second time I don't think they were ever using the "non
> > working way" in production but just to debug the other issue.
> 
> Thanks for explaining, then I think we can go for the "restrict
> position" fix which aligns with the -M, -t, ..., correct?

Hi Pablo,

To be frank, I think the 'restrict position' manner is really
unfriendly to users, which put burden on them to learn and remember this kind 
of rare and unique usage. I could create a bigger patch to change other
arugments '-M', '-t' along with '--concurrent'. Does this sound good to
you?

--
Firo

[PATCH 2/2] libebtc: Fix an issue that '--concurrent' doesn't work with NFS

[Firo Yang]

Due to the following commit[1] from kernel, if '/var/lib/ebtables' was
mounted with a NFS filesystem, ebtables command will hit the following
error:
mount | grep nfs
x.x.x.x:/var/lib/ebtables on /var/lib/ebtables type nfs4 [...]
/usr/sbin/ebtables --concurrent -L
Trying to obtain lock /var/lib/ebtables/lock
Trying to obtain lock /var/lib/ebtables/lock
Trying to obtain lock /var/lib/ebtables/lock
Trying to obtain lock /var/lib/ebtables/lock
[...]

In order to fix this problem, add 'O_WRONLY' to match the requirement of
that kernel commit[1].

[1]: 55725513b5ef ("NFSv4: Ensure that we check lock exclusive/shared type against open modes")

Signed-off-by: Firo Yang <firo.yang@suse.com>
---
 libebtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libebtc.c b/libebtc.c
index 1b058ef..112c307 100644
--- a/libebtc.c
+++ b/libebtc.c
@@ -144,7 +144,7 @@ static int lock_file()
 	int fd, try = 0;
 
 retry:
-	fd = open(LOCKFILE, O_CREAT|O_CLOEXEC, 00600);
+	fd = open(LOCKFILE, O_CREAT|O_WRONLY|O_CLOEXEC, 00600);
 	if (fd < 0) {
 		if (try == 1 || mkdir(dirname(pathbuf), 00700))
 			return -2;
-- 
2.30.2