Re: start unlocking kbind(2)
On Wed, Jun 15, 2022 at 06:17:07PM -0600, Theo de Raadt wrote:
> Mark Kettenis wrote:
>
> > Well, I believe that Scott was trying to fix a race condition that can
> > only happen if code is using kbind(2) incorrectly, i.e. when the
> > threads deliberately pass different cookies to kbind(2) or execute
> > kbind(2) from different "text" addresses.
> >
> > I still think the solution is simply to accept that race condition.
>
> Right.
>
> People are not calling kbind. They are calling syscall(SYS_kbind
>
> The man page says "don't do that". No user serviceable parts inside.
> Do not provide to children.
>
> That said, Scott is about to share a diff he and I did a few cycles
> around, to at least make the call-in transaction be a lock.
Okay, here it is.
This patch reorganizes the logic in sys_kbind() preceding the
copyin(9) so that we only need to take the kernel lock in a single
spot if something goes wrong and we need to raise SIGILL.
It also puts the per-process mutex, ps_mtx, around that logic. This
guarantees that the first thread to reach sys_kbind() sets
ps_kbind_addr and ps_kbind_cookie, even in oddball situations where
the program isn't using ld.so(1) correctly.
I am aware that this is "unsupported", but on balance I would prefer
that the initialization of those two variables was atomic. I think
taking the mutex is a very small price to pay for the guarantee that
the "security check" logic always happens the way it was intended to
happen in the order it was intended to happen. Note that we are not
wrapping the binding loop up in a lock, just the ps_kbind_* variable
logic.
... if Mark and Philip want to push back on that, I think I would
yield and just drop the mutex.
... also, if Philip goes ahead with the PT_OPENBSD_KBIND thing we no
longer need the mutex because, as I understand it, we would no longer
need the ps_kbind_cookie.
Even in either of those cases I still think the logic refactoring
shown here is a good thing.
--
I have been running with this now for a day and haven't hit any
panics. The last time I tried unlocking kbind(2) I hit panics pretty
quickly due to KERNEL_ASSERT_LOCKED() calls down in the binding loop.
Because I'm not seeing that I assume this means something has changed
down in UVM and we no longer need to wrap the binding loop with the
kernel lock.
Thoughts?
Index: uvm/uvm_mmap.c
===
RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
retrieving revision 1.169
diff -u -p -r1.169 uvm_mmap.c
--- uvm/uvm_mmap.c 19 Jan 2022 10:43:48 - 1.169
+++ uvm/uvm_mmap.c 16 Jun 2022 03:36:53 -
@@ -1127,7 +1127,7 @@ sys_kbind(struct proc *p, void *v, regis
size_t psize, s;
u_long pc;
int count, i, extra;
- int error;
+ int error, sigill = 0;
/*
* extract syscall args from uap
@@ -1135,23 +1135,41 @@ sys_kbind(struct proc *p, void *v, regis
paramp = SCARG(uap, param);
psize = SCARG(uap, psize);
- /* a NULL paramp disables the syscall for the process */
- if (paramp == NULL) {
- if (pr->ps_kbind_addr != 0)
- sigexit(p, SIGILL);
- pr->ps_kbind_addr = BOGO_PC;
- return 0;
- }
-
- /* security checks */
+ /*
+* If paramp is NULL and we're uninitialized, disable the syscall
+* for the process. Raise SIGILL if we're already initialized.
+*
+* If paramp is non-NULL and we're uninitialized, do initialization.
+* Otherwise, do security checks raise SIGILL on failure.
+*/
pc = PROC_PC(p);
- if (pr->ps_kbind_addr == 0) {
+ mtx_enter(>ps_mtx);
+ if (paramp == NULL) {
+ if (pr->ps_kbind_addr == 0)
+ pr->ps_kbind_addr = BOGO_PC;
+ else
+ sigill = 1;
+ } else if (pr->ps_kbind_addr == 0) {
pr->ps_kbind_addr = pc;
pr->ps_kbind_cookie = SCARG(uap, proc_cookie);
- } else if (pc != pr->ps_kbind_addr || pc == BOGO_PC)
- sigexit(p, SIGILL);
- else if (pr->ps_kbind_cookie != SCARG(uap, proc_cookie))
+ } else if (pc != pr->ps_kbind_addr || pc == BOGO_PC ||
+ pr->ps_kbind_cookie != SCARG(uap, proc_cookie)) {
+ sigill = 1;
+ }
+ mtx_leave(>ps_mtx);
+
+ /* Raise SIGILL if something is off. */
+ if (sigill) {
+ KERNEL_LOCK();
sigexit(p, SIGILL);
+ /* NOTREACHED */
+ KERNEL_UNLOCK();
+ }
+
+ /* We're done if we were disabling the syscall. */
+ if (paramp == NULL)
+ return 0;
+
if (psize < sizeof(struct __kbind) || psize > sizeof(param))
return EINVAL;
if ((error = copyin(paramp, , psize)))
Index: kern/syscalls.master
===
Re: start unlocking kbind(2)
Mark Kettenis wrote: > Well, I believe that Scott was trying to fix a race condition that can > only happen if code is using kbind(2) incorrectly, i.e. when the > threads deliberately pass different cookies to kbind(2) or execute > kbind(2) from different "text" addresses. > > I still think the solution is simply to accept that race condition. Right. People are not calling kbind. They are calling syscall(SYS_kbind The man page says "don't do that". No user serviceable parts inside. Do not provide to children. That said, Scott is about to share a diff he and I did a few cycles around, to at least make the call-in transaction be a lock.
Re: start unlocking kbind(2)
> From: Philip Guenther > Date: Wed, 15 Jun 2022 10:07:06 -0900 > > On Mon, 13 Jun 2022, Theo de Raadt wrote: > > Scott Cheloha wrote: > > > > Am I wrong that kbind is never called twice in the same address space? > > > > > > Isn't this exactly what happened the last time we tried this? > > > > Tried what? kbind has never been NOLOCK. > > Scott's referring to rev 1.237 of kern_fork.c, where we tried to require the > first use of kbind be before __tfork(2) was called. This blew up because > there's at least two ways to legitimately call pthread_create(3) before > doing your first lazy binding: > > 1) build with -znow, then dlopen() something not linked with -znow after > calling pthread_create() > > 2) pass address of pthread_create() to another function which calls > through the pointer, no special link options required (just need to > split up the _create and *funcptr enough that the compiler > won't optimize to a direct call) > > I've thought about how to possibly force a lazy resolution and haven't > thought up anything that wasn't unmaintainable, and probably > unimplementable. Well, I believe that Scott was trying to fix a race condition that can only happen if code is using kbind(2) incorrectly, i.e. when the threads deliberately pass different cookies to kbind(2) or execute kbind(2) from different "text" addresses. I still think the solution is simply to accept that race condition.
Re: start unlocking kbind(2)
On Mon, 13 Jun 2022, Theo de Raadt wrote:
> Scott Cheloha wrote:
> > > Am I wrong that kbind is never called twice in the same address space?
> >
> > Isn't this exactly what happened the last time we tried this?
>
> Tried what? kbind has never been NOLOCK.
Scott's referring to rev 1.237 of kern_fork.c, where we tried to require
the first use of kbind be before __tfork(2) was called. This blew up
because there's at least two ways to legitimately call pthread_create(3)
before doing your first lazy binding:
1) build with -znow, then dlopen() something not linked with -znow after
calling pthread_create()
2) pass address of pthread_create() to another function which calls
through the pointer, no special link options required (just need to
split up the _create and *funcptr enough that the compiler
won't optimize to a direct call)
I've thought about how to possibly force a lazy resolution and haven't
thought up anything that wasn't unmaintainable, and probably
unimplementable.
So, what could work reliably?
My first thought would be to have ld.so's _dl_boot() do the equivalent of
struct __kbind kb = {
.kb_addr = _syscall_in_dl_bind;
.kb_size = 0;
};
kbind(, sizeof kb, pcookie)
...after teaching the kernel to accept such a first call to kbind and set
pr->ps_kbind_* from them. That would permit the reimposing of the "first
kbind can't be after __tfork" restriction; is it indeed enough to permit
the ps_kbind_* members without locks?
Another idea is to discard the cookie restriction and just trust the
calling address to be enough. I mean, we trust it to be sufficient
for sigreturn and that's perhaps more powerful, no? If we're fine with
that, then how about giving ld.so a PT_OPENBSD_KBIND segment with the
required address, so the kernel can just set it up at exec time?
Eventually, we could disable kbind if said header wasn't present on the ELF
interpreter.
(In a groteque abuse of the ELF program header, we *could* keep the cookie
by, for example, having the PT_OPENBSD_KBIND p_vaddr/p_memsz identify the
location of the random cookie in ld.so memory and pass the kbind syscall
address via p_paddr. You are permitted to barf.)
Philip
Re: bgpd prevent infinite-loop in pfkey code
On Wed, Jun 15, 2022 at 04:34:41PM +0200, Claudio Jeker wrote:
> Found by accident. If the pfkey message failed (sadb_msg_errno is set)
> then the pfkey code ends in an infinite loop because the erroneous message
> is not removed from the queue.
>
> pfkey_read() PEEKS at the message and returns 0 since it is what we
> expect. pfkey_reply() realizes it is an error and returns -1 but the
> message is still in the buffer. Once we hit the poll loop the fd
> immediatly triggers since the message is still in the buffer and we call
> pfkey_read() which peeks at the message and returns 0 ...
>
> Fix is simple, need to flush the message out in the error case.
ok
> --
> :wq Claudio
>
> Index: pfkey.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 pfkey.c
> --- pfkey.c 15 Jun 2022 14:09:30 - 1.63
> +++ pfkey.c 15 Jun 2022 14:23:39 -
> @@ -469,6 +469,9 @@ pfkey_reply(int sd, uint32_t *spi)
> return (0);
> else {
> log_warn("pfkey");
> + /* discard error message */
> + if (read(sd, , sizeof(hdr)) == -1)
> + log_warn("pfkey read");
> return (-1);
> }
> }
>
bgpd prevent infinite-loop in pfkey code
Found by accident. If the pfkey message failed (sadb_msg_errno is set)
then the pfkey code ends in an infinite loop because the erroneous message
is not removed from the queue.
pfkey_read() PEEKS at the message and returns 0 since it is what we
expect. pfkey_reply() realizes it is an error and returns -1 but the
message is still in the buffer. Once we hit the poll loop the fd
immediatly triggers since the message is still in the buffer and we call
pfkey_read() which peeks at the message and returns 0 ...
Fix is simple, need to flush the message out in the error case.
--
:wq Claudio
Index: pfkey.c
===
RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
retrieving revision 1.63
diff -u -p -r1.63 pfkey.c
--- pfkey.c 15 Jun 2022 14:09:30 - 1.63
+++ pfkey.c 15 Jun 2022 14:23:39 -
@@ -469,6 +469,9 @@ pfkey_reply(int sd, uint32_t *spi)
return (0);
else {
log_warn("pfkey");
+ /* discard error message */
+ if (read(sd, , sizeof(hdr)) == -1)
+ log_warn("pfkey read");
return (-1);
}
}
Re: m4: gnu-m4 compat draft
On Wed, Jun 08, 2022 at 09:44:19PM +0200, Marc Espie wrote:
> Naddy told me about an app that wants a gnu-m4 extension that
> requires >9 arguments to macros.
>
> I wrote a very quick patch that seems to do the work. There are probably
> lots of kinks to work out, it's been very lightly tested.
> (in particular, I haven't looked at stuff like $* and friends yet, maybe
> they work, maybe they won't)
>
> But if anyone requires this type of functionality, I'd like them to chime
> in.
>
>
> Index: eval.c
> ===
> RCS file: /cvs/src/usr.bin/m4/eval.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 eval.c
> --- eval.c28 Jun 2019 05:35:34 - 1.78
> +++ eval.c8 Jun 2022 13:03:29 -
> @@ -40,6 +40,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -542,6 +543,21 @@ expand_macro(const char *argv[], int arg
> p++;
> p--; /* last character of defn */
> while (p > t) {
> + if (mimic_gnu && isdigit(*p)) {
> + const char *pos = p;
> + int mult = 1;
> + argno = 0;
> + while (isdigit(*pos) && pos > t) {
> + argno += mult * (*pos - '0');
> + mult *= 10;
> + pos--;
> + }
> + if (*pos == ARGFLAG && argno < argc -1) {
> + pbstr(argv[argno + 1]);
> + p = pos-1;
> + continue;
> + }
> + }
> if (*(p - 1) != ARGFLAG)
> PUSHBACK(*p);
> else {
>
>
That code appears to work just fine and we even have a trivial regress test.
In the mean time, naddy@ tells me that the port in question moved away from
using that specific gnu-m4 extension.
On one side, that code is 100% self-contained and safe for non gnu-m4 compat
mode. And it went through a full bulk build without any issue.
On the other side, I don't know if some other stuff might rely on it
eventually. Among other things it is quite possible someone wrote an autoconf
macro somewhere that actually requires over 9 parameters.
So the question is: do I shelve it, or commit it ? I don't really care all
that much either way, I would just love to close the subject.
Re: don't depend on pfkeyv2.h in portable bgpd code
On Wed, Jun 15, 2022 at 01:43:29PM +0200, Claudio Jeker wrote:
> So pfkeyv2.h defines managed to sneak into the portable part of bgpd.
> The fix was to just dump a pfkeyv2.h version into the -portable compat
> code and call it a day. This is bad since pfkeyv2.h is highly OS
> dependent.
>
> This diff cleans up the mess by introducing new enums for the various
> IPsec algorithms. With that the net/pfkeyv2.h include in bgpd.h can be
> removed. In session.h we just need to forward declare struct sadb_msg
> which is simple enough and no problem since the portable code only uses
> NULL as argument.
>
> In pfkey.c the BGPD algorithm defines need to be switched to SADB ones but
> that is done with simple helper functions.
ok tb
>
> Is anyone actively using the IPsec support in bgpd? Not many other systems
> seem to support BGP over IPsec. Also the supported algorithms are just aes
> and 3des-cbc which is probably no longer adequate.
> --
> :wq Claudio
>
> ? obj
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.429
> diff -u -p -r1.429 bgpd.h
> --- bgpd.h15 Jun 2022 10:10:03 - 1.429
> +++ bgpd.h15 Jun 2022 10:56:34 -
> @@ -26,7 +26,6 @@
> #include
> #include
> #include
> -#include
>
> #include
> #include
> @@ -329,6 +328,18 @@ enum auth_method {
> AUTH_IPSEC_IKE_AH
> };
>
> +enum auth_alg {
> + AUTH_AALG_NONE,
> + AUTH_AALG_SHA1HMAC,
> + AUTH_AALG_MD5HMAC,
> +};
> +
> +enum auth_enc_alg {
> + AUTH_EALG_NONE,
> + AUTH_EALG_3DESCBC,
> + AUTH_EALG_AES,
> +};
> +
> struct peer_auth {
> charmd5key[TCP_MD5_KEY_LEN];
> charauth_key_in[IPSEC_AUTH_KEY_LEN];
> @@ -338,13 +349,13 @@ struct peer_auth {
> uint32_tspi_in;
> uint32_tspi_out;
> enum auth_methodmethod;
> + enum auth_alg auth_alg_in;
> + enum auth_alg auth_alg_out;
> + enum auth_enc_alg enc_alg_in;
> + enum auth_enc_alg enc_alg_out;
> uint8_t md5key_len;
> - uint8_t auth_alg_in;
> - uint8_t auth_alg_out;
> uint8_t auth_keylen_in;
> uint8_t auth_keylen_out;
> - uint8_t enc_alg_in;
> - uint8_t enc_alg_out;
> uint8_t enc_keylen_in;
> uint8_t enc_keylen_out;
> };
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.429
> diff -u -p -r1.429 parse.y
> --- parse.y 9 Jun 2022 17:33:47 - 1.429
> +++ parse.y 15 Jun 2022 10:56:35 -
> @@ -193,7 +193,7 @@ typedef struct {
> struct filter_prefixlen prefixlen;
> struct prefixset_item *prefixset_item;
> struct {
> - uint8_t enc_alg;
> + enum auth_enc_alg enc_alg;
> uint8_t enc_key_len;
> charenc_key[IPSEC_ENC_KEY_LEN];
> } encspec;
> @@ -1609,7 +1609,7 @@ peeropts: REMOTEAS as4number{
> curpeer->conf.auth.method = AUTH_IPSEC_IKE_AH;
> }
> | IPSEC espah inout SPI NUMBER STRING STRING encspec {
> - uint32_tauth_alg;
> + enum auth_alg auth_alg;
> uint8_t keylen;
>
> if (curpeer->conf.auth.method &&
> @@ -1626,10 +1626,10 @@ peeropts : REMOTEAS as4number{
> }
>
> if (!strcmp($6, "sha1")) {
> - auth_alg = SADB_AALG_SHA1HMAC;
> + auth_alg = AUTH_AALG_SHA1HMAC;
> keylen = 20;
> } else if (!strcmp($6, "md5")) {
> - auth_alg = SADB_AALG_MD5HMAC;
> + auth_alg = AUTH_AALG_MD5HMAC;
> keylen = 16;
> } else {
> yyerror("unknown auth algorithm \"%s\"", $6);
> @@ -1860,11 +1860,11 @@ encspec : /* nada */{
> | STRING STRING {
> bzero(&$$, sizeof($$));
> if (!strcmp($1, "3des") || !strcmp($1, "3des-cbc")) {
> - $$.enc_alg = SADB_EALG_3DESCBC;
> + $$.enc_alg = AUTH_EALG_3DESCBC;
> $$.enc_key_len = 21; /* XXX verify */
> } else if (!strcmp($1, "aes") ||
> !strcmp($1, "aes-128-cbc")) {
> -
don't depend on pfkeyv2.h in portable bgpd code
So pfkeyv2.h defines managed to sneak into the portable part of bgpd.
The fix was to just dump a pfkeyv2.h version into the -portable compat
code and call it a day. This is bad since pfkeyv2.h is highly OS
dependent.
This diff cleans up the mess by introducing new enums for the various
IPsec algorithms. With that the net/pfkeyv2.h include in bgpd.h can be
removed. In session.h we just need to forward declare struct sadb_msg
which is simple enough and no problem since the portable code only uses
NULL as argument.
In pfkey.c the BGPD algorithm defines need to be switched to SADB ones but
that is done with simple helper functions.
Is anyone actively using the IPsec support in bgpd? Not many other systems
seem to support BGP over IPsec. Also the supported algorithms are just aes
and 3des-cbc which is probably no longer adequate.
--
:wq Claudio
? obj
Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.429
diff -u -p -r1.429 bgpd.h
--- bgpd.h 15 Jun 2022 10:10:03 - 1.429
+++ bgpd.h 15 Jun 2022 10:56:34 -
@@ -26,7 +26,6 @@
#include
#include
#include
-#include
#include
#include
@@ -329,6 +328,18 @@ enum auth_method {
AUTH_IPSEC_IKE_AH
};
+enum auth_alg {
+ AUTH_AALG_NONE,
+ AUTH_AALG_SHA1HMAC,
+ AUTH_AALG_MD5HMAC,
+};
+
+enum auth_enc_alg {
+ AUTH_EALG_NONE,
+ AUTH_EALG_3DESCBC,
+ AUTH_EALG_AES,
+};
+
struct peer_auth {
charmd5key[TCP_MD5_KEY_LEN];
charauth_key_in[IPSEC_AUTH_KEY_LEN];
@@ -338,13 +349,13 @@ struct peer_auth {
uint32_tspi_in;
uint32_tspi_out;
enum auth_methodmethod;
+ enum auth_alg auth_alg_in;
+ enum auth_alg auth_alg_out;
+ enum auth_enc_alg enc_alg_in;
+ enum auth_enc_alg enc_alg_out;
uint8_t md5key_len;
- uint8_t auth_alg_in;
- uint8_t auth_alg_out;
uint8_t auth_keylen_in;
uint8_t auth_keylen_out;
- uint8_t enc_alg_in;
- uint8_t enc_alg_out;
uint8_t enc_keylen_in;
uint8_t enc_keylen_out;
};
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.429
diff -u -p -r1.429 parse.y
--- parse.y 9 Jun 2022 17:33:47 - 1.429
+++ parse.y 15 Jun 2022 10:56:35 -
@@ -193,7 +193,7 @@ typedef struct {
struct filter_prefixlen prefixlen;
struct prefixset_item *prefixset_item;
struct {
- uint8_t enc_alg;
+ enum auth_enc_alg enc_alg;
uint8_t enc_key_len;
charenc_key[IPSEC_ENC_KEY_LEN];
} encspec;
@@ -1609,7 +1609,7 @@ peeropts : REMOTEAS as4number{
curpeer->conf.auth.method = AUTH_IPSEC_IKE_AH;
}
| IPSEC espah inout SPI NUMBER STRING STRING encspec {
- uint32_tauth_alg;
+ enum auth_alg auth_alg;
uint8_t keylen;
if (curpeer->conf.auth.method &&
@@ -1626,10 +1626,10 @@ peeropts: REMOTEAS as4number{
}
if (!strcmp($6, "sha1")) {
- auth_alg = SADB_AALG_SHA1HMAC;
+ auth_alg = AUTH_AALG_SHA1HMAC;
keylen = 20;
} else if (!strcmp($6, "md5")) {
- auth_alg = SADB_AALG_MD5HMAC;
+ auth_alg = AUTH_AALG_MD5HMAC;
keylen = 16;
} else {
yyerror("unknown auth algorithm \"%s\"", $6);
@@ -1860,11 +1860,11 @@ encspec : /* nada */{
| STRING STRING {
bzero(&$$, sizeof($$));
if (!strcmp($1, "3des") || !strcmp($1, "3des-cbc")) {
- $$.enc_alg = SADB_EALG_3DESCBC;
+ $$.enc_alg = AUTH_EALG_3DESCBC;
$$.enc_key_len = 21; /* XXX verify */
} else if (!strcmp($1, "aes") ||
!strcmp($1, "aes-128-cbc")) {
- $$.enc_alg = SADB_X_EALG_AES;
+ $$.enc_alg = AUTH_EALG_AES;
$$.enc_key_len = 16;
} else {
Re: [v2] amd64: simplify TSC sync testing
Hi Scott, just installing on another 2-socket machine, could you point me at the latest version of the TSC sync testing diff please?
Re: bgpd more kroute cleanup
On Tue, Jun 14, 2022 at 08:14:45PM +0200, Theo Buehler wrote:
> On Tue, Jun 14, 2022 at 06:36:13PM +0200, Claudio Jeker wrote:
> > This diff does introduce a new flag for routes that have been inserted
> > into the FIB. Now I actually renamed the F_BGPD_INSERTED flag to F_BGPD
> > and reused F_BGPD_INSERTED as this new flag.
> > Adjust and use the send_rtmsg() return value to set F_BGPD_INSERTED if the
> > route message was successfully sent.
> >
> > Additionally this removes F_DYNAMIC and tracking of dynamic routes (routes
> > generated by the kernel because of PMTU or some other event). A routing
> > protocol does not need to work with them so just filter them out like ARP
> > and ND routes.
> >
> > While there also just remove protect_lo() it is a function that is not
> > really making anything better. There are other checks in place for
> > 127.0.0.1 and ::1.
>
> Would it not be cleaner to set or unset the F_BGPD_INSERTED flag
> inside send_rt{,6}msg() depending on action instead of doing that
> after inspecting the return code?
I decided against this because send_rtmsg() should not alter the kroute
node. In the end I want the kroute code to be a module and have the rtsock
code to be just a few hooks. This should make the code a lot more portable
with far less maintenance work.
> ok tb
>
> >
> > --
> > :wq Claudio
> >
> > Index: bgpctl/bgpctl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > retrieving revision 1.276
> > diff -u -p -r1.276 bgpctl.c
> > --- bgpctl/bgpctl.c 21 Mar 2022 10:16:23 - 1.276
> > +++ bgpctl/bgpctl.c 14 Jun 2022 15:31:59 -
> > @@ -633,14 +633,12 @@ fmt_fib_flags(uint16_t flags)
> > else
> > strlcpy(buf, "*", sizeof(buf));
> >
> > - if (flags & F_BGPD_INSERTED)
> > + if (flags & F_BGPD)
> > strlcat(buf, "B", sizeof(buf));
> > else if (flags & F_CONNECTED)
> > strlcat(buf, "C", sizeof(buf));
> > else if (flags & F_STATIC)
> > strlcat(buf, "S", sizeof(buf));
> > - else if (flags & F_DYNAMIC)
> > - strlcat(buf, "D", sizeof(buf));
> > else
> > strlcat(buf, " ", sizeof(buf));
> >
> > Index: bgpctl/output.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> > retrieving revision 1.20
> > diff -u -p -r1.20 output.c
> > --- bgpctl/output.c 6 Feb 2022 09:52:32 - 1.20
> > +++ bgpctl/output.c 14 Jun 2022 15:51:49 -
> > @@ -46,7 +46,7 @@ show_head(struct parse_result *res)
> > break;
> > case SHOW_FIB:
> > printf("flags: * = valid, B = BGP, C = Connected, "
> > - "S = Static, D = Dynamic\n");
> > + "S = Static\n");
> > printf(" "
> > "N = BGP Nexthop reachable via this route\n");
> > printf(" r = reject route, b = blackhole route\n\n");
> > Index: bgpctl/output_json.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 output_json.c
> > --- bgpctl/output_json.c21 Mar 2022 10:16:23 - 1.14
> > +++ bgpctl/output_json.c14 Jun 2022 15:32:05 -
> > @@ -357,14 +357,12 @@ json_fib(struct kroute_full *kf)
> > json_do_printf("prefix", "%s/%u", log_addr(>prefix), kf->prefixlen);
> > json_do_uint("priority", kf->priority);
> > json_do_bool("up", !(kf->flags & F_DOWN));
> > - if (kf->flags & F_BGPD_INSERTED)
> > + if (kf->flags & F_BGPD)
> > origin = "bgp";
> > else if (kf->flags & F_CONNECTED)
> > origin = "connected";
> > else if (kf->flags & F_STATIC)
> > origin = "static";
> > - else if (kf->flags & F_DYNAMIC)
> > - origin = "dynamic";
> > else
> > origin = "unknown";
> > json_do_printf("origin", "%s", origin);
> > Index: bgpctl/parser.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> > retrieving revision 1.109
> > diff -u -p -r1.109 parser.c
> > --- bgpctl/parser.c 21 Mar 2022 10:16:23 - 1.109
> > +++ bgpctl/parser.c 14 Jun 2022 15:29:05 -
> > @@ -151,15 +151,15 @@ static const struct token t_show_summary
> > };
> >
> > static const struct token t_show_fib[] = {
> > - { NOTOKEN, "", NONE,NULL},
> > - { FLAG, "connected",F_CONNECTED, t_show_fib},
> > - { FLAG, "static", F_STATIC,t_show_fib},
> > - { FLAG, "bgp", F_BGPD_INSERTED, t_show_fib},
> > - { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
> > - { KEYWORD, "table",NONE,t_show_fib_table},
> > - { FAMILY, "", NONE,t_show_fib},
> > - { ADDRESS, "", NONE,
Re: [PATCH 1/1] drm/amdgpu: Fix double free of dmabuf
As mentioned earlier, you should send these patches to the linux maintainers. We don't even build this file. On Wed, Jun 15, 2022 at 11:29:31AM +0800, Xiaohui Zhang wrote: > amdgpu_amdkfd_gpuvm_free_memory_of_gpu drop dmabuf reference increased in > amdgpu_gem_prime_export. > amdgpu_bo_destroy drop dmabuf reference increased in > amdgpu_gem_prime_import. > > So remove this extra dma_buf_put to avoid double free. > > Signed-off-by: Xiaohui Zhang > --- > sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 -- > 1 file changed, 6 deletions(-) > > diff --git a/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > b/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > index 34bd9712d..00c311082 100644 > --- a/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > +++ b/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > @@ -644,12 +644,6 @@ kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct > kgd_mem *mem, > if (IS_ERR(gobj)) > return PTR_ERR(gobj); > > - /* Import takes an extra reference on the dmabuf. Drop it now to > - * avoid leaking it. We only need the one reference in > - * kgd_mem->dmabuf. > - */ > - dma_buf_put(mem->dmabuf); > - > *bo = gem_to_amdgpu_bo(gobj); > (*bo)->flags |= AMDGPU_GEM_CREATE_PREEMPTIBLE; > (*bo)->parent = amdgpu_bo_ref(mem->bo); > -- > 2.17.1 > >
[PATCH 1/1] drm/amdgpu: Fix double free of dmabuf
amdgpu_amdkfd_gpuvm_free_memory_of_gpu drop dmabuf reference increased in amdgpu_gem_prime_export. amdgpu_bo_destroy drop dmabuf reference increased in amdgpu_gem_prime_import. So remove this extra dma_buf_put to avoid double free. Signed-off-by: Xiaohui Zhang --- sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 34bd9712d..00c311082 100644 --- a/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/sys/dev/pci/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -644,12 +644,6 @@ kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem, if (IS_ERR(gobj)) return PTR_ERR(gobj); - /* Import takes an extra reference on the dmabuf. Drop it now to -* avoid leaking it. We only need the one reference in -* kgd_mem->dmabuf. -*/ - dma_buf_put(mem->dmabuf); - *bo = gem_to_amdgpu_bo(gobj); (*bo)->flags |= AMDGPU_GEM_CREATE_PREEMPTIBLE; (*bo)->parent = amdgpu_bo_ref(mem->bo); -- 2.17.1
