netdev - Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Nov 2020 15:06:16 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Tom Parkin <tparkin@...alix.com>
Cc:     netdev@...r.kernel.org, jchapman@...alix.com
Subject: Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

On Tue, Nov 17, 2020 at 12:26:38PM +0000, Tom Parkin wrote:
> On  Sun, Nov 15, 2020 at 17:28:38 +0100, Guillaume Nault wrote:
> > On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> > > On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > > However, my primary motivation for using ppp_channel_push was actually
> > > the handling for managing dropping the packet if the channel was
> > > deregistered.
> > 
> > I might be missing something, but I don't see what ppp_channel_push()
> > does appart from holding the xmit lock and handling the xmit queue.
> > If we agree that there's no need to use the xmit queue, all
> > ppp_channel_push() does for us is taking pch->downl, which we probably
> > can do on our own.
> > 
> > > It'd be simple enough to add another function which performed the same
> > > deregistration check but didn't transmit via. the queue.
> > 
> > That's probably what I'm missing: what do you mean by "deregistration
> > check"? I can't see anything like this in ppp_channel_push().
> 
> It's literally just the check on pch->chan once pch->downl is held.
> So it would be trivial to do the same thing in a different codepath: I
> just figured why reinvent the wheel :-)

Okay, I was thinking of something more complex. I agree with not
reinventing existing functions, but in this case, I think that
ppp_channel_push() does too many unecessary operations (like recursion
handling and processing the parent unit's queue). Also, a helper
function would be the natural place for calling skb_scrub_packet()
and for handling concurent access or modification of the ->bridge
pointer (as discussed earlier in this thread).

> 
> Sorry for the confusion.

No problem. It's nice to see some work being done in this area :).