Re: [iptables PATCH 2/2] nfnl_osf: Improve error handling

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Tue, May 12, 2020 at 02:49:49PM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 11, 2020 at 01:31:12PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Sat, May 09, 2020 at 07:28:07PM +0200, Pablo Neira Ayuso wrote:
> > > On Sat, May 09, 2020 at 01:52:00PM +0200, Phil Sutter wrote:
> > > > For some error cases, no log message was created - hence apart from the
> > > > return code there was no indication of failing execution.
> > > > 
> > > > When loading a line fails, don't abort but continue with the remaining
> > > > file contents. The current pf.os file in this repository serves as
> > > > proof-of-concept: Loading all entries succeeds, but when deleting, lines
> > > > 700, 701 and 704 return ENOENT. Not continuing means the remaining
> > > > entries are not cleared.
> > > 
> > > Did you look at why are these lines returning ENOENT?
> > 
> > If I understand the code right, line 700 is a duplicate of line 698, 701
> > of 699 and 704 of 702. This is because 'W*' parses identical to 'W0' and
> > in right-hand side only the first three text fields (genre, version and
> > subtype) are relevant - the rest is ignored.
> 
> I see, in the userspace parser, W0 and W* are being handled as
> OSF_WSS_PLAIN.
> 
> > When adding, this doesn't become visible because flag NLM_F_EXCL is not
> > specified. If it is, kernel returns EEXISTS for those lines.
> 
> In the kernel, the struct nf_osf_user_finger is used as key to
> identify each line, given they are identical.
> 
> So it looks like this EEXIST has been there since the beginning.
> 
> This patchset LGTM, it's just that the user might get confused if it
> see errors when using this tool, probably turning this into a warning
> is fine.

Yes, at least it's unfortunate that the default fingerprint file
triggers them. We could drop the offending lines, but then again re-sync
with OpenBSD won't be trivial anymore.

From my PoV we may also just ignore the error conditions. Most important
bit here is to not stop on error, at least not when deleting.

> Or at least, include this information in the commit message so this
> does not get lost :-)

Yes, I'll extend the commit message. Thanks for the reminder.

Cheers, Phil